| Message ID | 20200613150751.114595-1-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] i2c: xiic: Fix broken locking on tx_msg | expand |
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
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. [...]
> -----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. > > [...]
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 --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;
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(-)