Hi Marek,
Thanks for the patches. I have tested them. Please see comments inline.
> -----Original Message-----
> From: linux-i2c-owner@xxxxxxxxxxxxxxx <linux-i2c-owner@xxxxxxxxxxxxxxx> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@xxxxxxxxxxxxxxx
> Cc: Marek Vasut <marex@xxxxxxx>; Michal Simek <michals@xxxxxxxxxx>;
> Shubhrajyoti Datta <shubhraj@xxxxxxxxxx>; Wolfram Sang <wsa@xxxxxxxxxx>
> Subject: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
>
> The tx_msg is set from multiple places, sometimes without locking, which fall
> apart on any SMP system. Only ever access tx_msg inside the driver mutex.
>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> Cc: Michal Simek <michal.simek@xxxxxxxxxx>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xxxxxxxxxx>
> Cc: Wolfram Sang <wsa@xxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-xiic.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-xiic.c b/drivers/i2c/busses/i2c-xiic.c index
> 90c1c362394d0..0777e577720db 100644
> --- a/drivers/i2c/busses/i2c-xiic.c
> +++ b/drivers/i2c/busses/i2c-xiic.c
> @@ -168,7 +168,7 @@ struct xiic_i2c {
> #define xiic_tx_space(i2c) ((i2c)->tx_msg->len - (i2c)->tx_pos) #define
> xiic_rx_space(i2c) ((i2c)->rx_msg->len - (i2c)->rx_pos)
>
> -static int xiic_start_xfer(struct xiic_i2c *i2c);
> +static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs,
> +int num);
> static void __xiic_start_xfer(struct xiic_i2c *i2c);
>
> /*
> @@ -673,15 +673,24 @@ static void __xiic_start_xfer(struct xiic_i2c *i2c)
>
> }
>
> -static int xiic_start_xfer(struct xiic_i2c *i2c)
> +static int xiic_start_xfer(struct xiic_i2c *i2c, struct i2c_msg *msgs,
> +int num)
> {
> int ret;
> +
> mutex_lock(&i2c->lock);
>
> + ret = xiic_busy(i2c);
> + if (ret)
> + goto out;
> +
> + i2c->tx_msg = msgs;
> + i2c->nmsgs = num;
> +
> ret = xiic_reinit(i2c);
> if (!ret)
> __xiic_start_xfer(i2c);
>
> +out:
> mutex_unlock(&i2c->lock);
>
> return ret;
> @@ -699,14 +708,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
> if (err < 0)
> return err;
>
> - err = xiic_busy(i2c);
> - if (err)
> - goto out;
> -
> - i2c->tx_msg = msgs;
> - i2c->nmsgs = num;
On an SMP system with multiple i2c-transfer command scripts running, the above critical section is expected to cause serious trouble overwriting the previous msg pointers.
But that's not happening as the i2c-core is having a lock at adapter level inside i2c-core-base.c (rt_mutex_lock_nested).
So, the race condition between different threads is not possible. They are all serialized by the locking in i2c-core.
Although no issues are seen in the tests, the contention within the driver is still possible (isr vs xiic_xfer), if there is a spurious interrupt. And this patch is needed in that case.
> -
> - err = xiic_start_xfer(i2c);
> + err = xiic_start_xfer(i2c, msgs, num);
> if (err < 0) {
> dev_err(adap->dev.parent, "Error xiic_start_xfer\n");
> goto out;
> @@ -714,9 +716,11 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
>
> if (wait_event_timeout(i2c->wait, (i2c->state == STATE_ERROR) ||
> (i2c->state == STATE_DONE), HZ)) {
> + mutex_lock(&i2c->lock);
> err = (i2c->state == STATE_DONE) ? num : -EIO;
> goto out;
> } else {
> + mutex_lock(&i2c->lock);
> i2c->tx_msg = NULL;
> i2c->rx_msg = NULL;
> i2c->nmsgs = 0;
> @@ -724,6 +728,7 @@ static int xiic_xfer(struct i2c_adapter *adap, struct
> i2c_msg *msgs, int num)
> goto out;
> }
> out:
> + mutex_unlock(&i2c->lock);
> pm_runtime_mark_last_busy(i2c->dev);
> pm_runtime_put_autosuspend(i2c->dev);
> return err;
Raviteja N
