NACK: [SRU][F:linux-bluefield][PATCH v3 1/1] UBUNTU: SAUCE: i2c-mlxbf.c: support lock mechanism
Zachary Tahenakos
zachary.tahenakos at canonical.com
Thu Jul 21 13:45:45 UTC 2022
I'm talking about this here:
if (flags & I2C_F_WRITE) {
write_en = 1;
write_len += operation->length;
if (data_idx + operation->length >
MASTER_DATA_DESC_SIZE)
return -ENOBUFS;
memcpy(data_desc + data_idx,
operation->buffer, operation->length);
data_idx += operation->length;
}
This is part of the for loop shortly after setting the lock. There is a
conditional return that does not clear the lock before returning.
On Thu, Jul 21, 2022 at 9:38 AM Asmaa Mnebhi <asmaa at nvidia.com> wrote:
> I am not sure I understand the question. I addressed Tim’s comment and
> released the lock in the second loop exit point.
>
>
> + if (WARN_ON(!mlx_smbus_master_lock(priv)))
> + return -EBUSY;
> +
> + /* Check whether the HW is idle */
> + if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) {
> + mlx_smbus_master_unlock(priv);
> return -EBUSY;
> + }
>
>
> *From:* Zachary Tahenakos <zachary.tahenakos at canonical.com>
> *Sent:* Thursday, July 21, 2022 9:35 AM
> *To:* Asmaa Mnebhi <asmaa at nvidia.com>
> *Cc:* kernel-team at lists.ubuntu.com
> *Subject:* NACK: [SRU][F:linux-bluefield][PATCH v3 1/1] UBUNTU: SAUCE:
> i2c-mlxbf.c: support lock mechanism
>
>
>
> There is an exit point within the modified function after the lock has
> been set but the lock isn't being released if we return at this point. This
> return is within the for loop following the setting of the lock bit.
>
>
>
> On Wed, Jul 20, 2022 at 9:38 AM Asmaa Mnebhi <asmaa at nvidia.com> wrote:
>
> Buglink: https://bugs.launchpad.net/bugs/1981105
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.launchpad.net%2Fbugs%2F1981105&data=05%7C01%7Casmaa%40nvidia.com%7Cc84f4aa7261740ed1bc308da6b1dda64%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637940073221061486%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jQJIAEetDruX%2FDpXESCdyswqH4dM8nhuhmQ%2F50CKDmE%3D&reserved=0>
>
> Support the I2C lock mechanism, otherwise there could be
> unexpected behavior when an i2c bus is accessed by
> several entities like the linux driver, ATF driver and UEFI driver.
>
> Make sure to pick up the ATF/UEFI image to accompany this change
> because at boot time ATF will ensure that the lock is released.
>
> Signed-off-by: Asmaa Mnebhi <asmaa at nvidia.com>
> ---
> drivers/i2c/busses/i2c-mlxbf.c | 40 ++++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mlxbf.c
> b/drivers/i2c/busses/i2c-mlxbf.c
> index 0ac9e70244a7..b951443b91f8 100644
> --- a/drivers/i2c/busses/i2c-mlxbf.c
> +++ b/drivers/i2c/busses/i2c-mlxbf.c
> @@ -340,7 +340,8 @@ enum {
> * Timeout is given in microsends. Note also that timeout handling is not
> * exact.
> */
> -#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */
> +#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */
> +#define SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */
>
> /* Encapsulates timing parameters */
> struct mlx_i2c_timings {
> @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct
> mlx_i2c_priv *priv)
> return false;
> }
>
> +/*
> + * wait for the lock to be released before acquiring it.
> + */
> +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv)
> +{
> + if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW,
> + 1 << MASTER_LOCK_BIT_OFF, true,
> + SMBUS_LOCK_POLL_TIMEOUT))
> + return true;
> +
> + return false;
> +}
> +
> +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv)
> +{
> + /* Clear the gw to clear the lock */
> + writel(0, priv->smbus->io + SMBUS_MASTER_GW);
> +}
> +
> /*
> * Poll SMBus master status and return transaction status,
> * i.e. whether succeeded or failed. I2C and SMBus fault codes
> @@ -744,9 +764,17 @@ static int mlx_smbus_start_transaction(struct
> mlx_i2c_priv *priv,
> slave = request->slave & 0x7f;
> addr = slave << 1;
>
> - /* First of all, check whether the HW is idle */
> - if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv)))
> + /* Try to acquire the smbus gw lock before any reads of the GW
> register since
> + * a read sets the lock.
> + */
> + if (WARN_ON(!mlx_smbus_master_lock(priv)))
> + return -EBUSY;
> +
> + /* Check whether the HW is idle */
> + if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) {
> + mlx_smbus_master_unlock(priv);
> return -EBUSY;
> + }
>
> /* Set first byte */
> data_desc[data_idx++] = addr;
> @@ -802,8 +830,10 @@ static int mlx_smbus_start_transaction(struct
> mlx_i2c_priv *priv,
> if (write_en) {
> ret = mlx_smbus_enable(priv, slave, write_len, block_en,
> pec_en, 0);
> - if (ret != 0)
> + if (ret) {
> + mlx_smbus_master_unlock(priv);
> return ret;
> + }
> }
>
> if (read_en) {
> @@ -830,6 +860,8 @@ static int mlx_smbus_start_transaction(struct
> mlx_i2c_priv *priv,
> SMBUS_MASTER_FSM_PS_STATE_MASK);
> }
>
> + mlx_smbus_master_unlock(priv);
> +
> return ret;
> }
>
> --
> 2.30.1
>
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.ubuntu.com%2Fmailman%2Flistinfo%2Fkernel-team&data=05%7C01%7Casmaa%40nvidia.com%7Cc84f4aa7261740ed1bc308da6b1dda64%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637940073221061486%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=HyHm5a7B%2F%2F%2F3OW2UxXLlS3schQSsZ%2BZ4XWAZj3XSgcE%3D&reserved=0>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20220721/c797373e/attachment.html>
More information about the kernel-team
mailing list