diff mbox series

[1/5] i2c: xiic: Fix broken locking on tx_msg

Message ID 20200613150751.114595-1-marex@denx.de
State New
Headers show
Series [1/5] i2c: xiic: Fix broken locking on tx_msg | expand

Commit Message

Marek Vasut June 13, 2020, 3:07 p.m. UTC
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@denx.de>
Cc: Michal Simek <michal.simek@xilinx.com>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Wolfram Sang <wsa@kernel.org>
---
 drivers/i2c/busses/i2c-xiic.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

Comments

Raviteja Narayanam June 26, 2020, 12:11 p.m. UTC | #1
Hi Marek,

Thanks for the patches. I have tested them. Please see comments inline.

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Marek Vasut
> Sent: Saturday, June 13, 2020 8:38 PM
> To: linux-i2c@vger.kernel.org
> Cc: Marek Vasut <marex@denx.de>; Michal Simek <michals@xilinx.com>;
> Shubhrajyoti Datta <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> 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@denx.de>
> Cc: Michal Simek <michal.simek@xilinx.com>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Wolfram Sang <wsa@kernel.org>
> ---
>  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
Marek Vasut June 28, 2020, 11:18 p.m. UTC | #2
On 6/26/20 2:11 PM, Raviteja Narayanam wrote:

Hi,

[...]

>> @@ -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.
The contention happens between the threaded interrupt handler
xiic_process() and this xiic_xfer() function. While you can not have
xiic_xfer() running on two CPU cores at the same time, you can have
xiic_xfer() running on one CPU core and xiic_process() on another CPU
core, and that will lead to problems.

[...]
Raviteja Narayanam June 29, 2020, 12:52 p.m. UTC | #3
> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, June 29, 2020 4:49 AM
> To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: Re: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> 
> On 6/26/20 2:11 PM, Raviteja Narayanam wrote:
> 
> Hi,
> 
> [...]
> 
> >> @@ -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.
> The contention happens between the threaded interrupt handler
> xiic_process() and this xiic_xfer() function. While you can not have
> xiic_xfer() running on two CPU cores at the same time, you can have
> xiic_xfer() running on one CPU core and xiic_process() on another CPU core,
> and that will lead to problems.

Yes, this patch is needed for that case.

> 
> [...]
Raviteja Narayanam July 8, 2020, 3:23 p.m. UTC | #4
Tested-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

Thanks
Raviteja N

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org> On
> Behalf Of Raviteja Narayanam
> Sent: Monday, June 29, 2020 6:22 PM
> To: Marek Vasut <marex@denx.de>; linux-i2c@vger.kernel.org
> Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> Subject: RE: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> 
> 
> 
> > -----Original Message-----
> > From: Marek Vasut <marex@denx.de>
> > Sent: Monday, June 29, 2020 4:49 AM
> > To: Raviteja Narayanam <rna@xilinx.com>; linux-i2c@vger.kernel.org
> > Cc: Michal Simek <michals@xilinx.com>; Shubhrajyoti Datta
> > <shubhraj@xilinx.com>; Wolfram Sang <wsa@kernel.org>
> > Subject: Re: [PATCH 1/5] i2c: xiic: Fix broken locking on tx_msg
> >
> > On 6/26/20 2:11 PM, Raviteja Narayanam wrote:
> >
> > Hi,
> >
> > [...]
> >
> > >> @@ -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.
> > The contention happens between the threaded interrupt handler
> > xiic_process() and this xiic_xfer() function. While you can not have
> > xiic_xfer() running on two CPU cores at the same time, you can have
> > xiic_xfer() running on one CPU core and xiic_process() on another CPU
> > core, and that will lead to problems.
> 
> Yes, this patch is needed for that case.
> 
> >
> > [...]
diff mbox series

Patch

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;
-
-	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;