diff mbox series

i2c: cadence: Handle transfer_size rollover

Message ID 20190131213957.11568-1-alex.williams@ettus.com (mailing list archive)
State Mainlined
Commit bbf967b223b3f1b55eb494d735226152afbad64e
Headers show
Series i2c: cadence: Handle transfer_size rollover | expand

Commit Message

Alex Williams Jan. 31, 2019, 9:39 p.m. UTC
From: Alex Williams <alex.williams@ni.com>

Under certain conditions, Cadence's I2C controller's transfer_size
register will roll over and generate invalid read transactions. Before
this change, the ISR relied solely on the RXDV bit to determine when to
write more data to the user's buffer. The invalid read data would cause
overruns, smashing stacks and worse.

This change stops the buffer writes to the requested boundary and
reports the error. The controller will be reset so normal transactions
may resume.

Signed-off-by: Alex Williams <alex.williams@ni.com>
---
 drivers/i2c/busses/i2c-cadence.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Alex Williams Feb. 15, 2019, 3:29 p.m. UTC | #1
On Fri, Feb 15, 2019 at 2:53 AM Shubhrajyoti Datta
<shubhrajyoti.datta@gmail.com> wrote:
>
> HI Alex,
>
> Thanks for the patch.
>
> On Fri, Feb 1, 2019 at 4:22 AM <alex.williams@ettus.com> wrote:
> >
> > From: Alex Williams <alex.williams@ni.com>
> >
> > Under certain conditions, Cadence's I2C controller's transfer_size
>
> Any help in reproducing the conditions would be appreciated
>
>
> > register will roll over and generate invalid read transactions. Before
> > this change, the ISR relied solely on the RXDV bit to determine when to
> > write more data to the user's buffer. The invalid read data would cause
> > overruns, smashing stacks and worse.
> >
> > This change stops the buffer writes to the requested boundary and
> > reports the error. The controller will be reset so normal transactions
> > may resume.
> >
> > Signed-off-by: Alex Williams <alex.williams@ni.com>


One possible related errata is here:
https://www.xilinx.com/support/answers/61664.html

In our case, we only needed to hammer on i2c to reproduce in a few
minutes, with a script like this:
while true
    do date
    cat /sys/class/gpio/gpio882/direction > /dev/null
    cat /sys/class/gpio/gpio883/direction > /dev/null
    cat /sys/class/gpio/gpio884/direction > /dev/null
    cat /sys/class/gpio/gpio885/direction > /dev/null
    cat /sys/class/gpio/gpio886/direction > /dev/null
    cat /sys/class/gpio/gpio887/direction > /dev/null
    cat /sys/class/gpio/gpio888/direction > /dev/null
    cat /sys/class/gpio/gpio889/direction > /dev/null
    cat /sys/class/gpio/gpio890/direction > /dev/null
    cat /sys/class/gpio/gpio891/direction > /dev/null
    cat /sys/class/gpio/gpio892/direction > /dev/null

    cat /sys/class/gpio/gpio894/direction > /dev/null
    cat /sys/class/gpio/gpio895/direction > /dev/null
    cat /sys/class/gpio/gpio896/direction > /dev/null
    cat /sys/class/gpio/gpio897/direction > /dev/null
    cat /sys/class/gpio/gpio898/direction > /dev/null
    cat /sys/class/gpio/gpio899/direction > /dev/null
    cat /sys/class/gpio/gpio900/direction > /dev/null
    cat /sys/class/gpio/gpio901/direction > /dev/null
    cat /sys/class/gpio/gpio902/direction > /dev/null
    cat /sys/class/gpio/gpio903/direction > /dev/null
    cat /sys/class/gpio/gpio904/direction > /dev/null
    cat /sys/class/gpio/gpio905/direction > /dev/null
done

In normal usage, we have code that sets up a number of i2c GPIO
expanders and pokes them for values as it initializes devices.
Occasionally, the transfer size register will roll over, and the ISR
will cause memory corruption, since it doesn't stop writing at the
requested boundary.
Shubhrajyoti Datta Nov. 28, 2019, 11:54 a.m. UTC | #2
Hi ,
Apologies for teh late reply,
Somehow replied only to Alex.
On Fri, Feb 15, 2019 at 8:59 PM Alex Williams <alex.williams@ettus.com> wrote:
>
> On Fri, Feb 15, 2019 at 2:53 AM Shubhrajyoti Datta
> <shubhrajyoti.datta@gmail.com> wrote:
> >
> > HI Alex,
> >
> > Thanks for the patch.
> >
> > On Fri, Feb 1, 2019 at 4:22 AM <alex.williams@ettus.com> wrote:
> > >
> > > From: Alex Williams <alex.williams@ni.com>
> > >
> > > Under certain conditions, Cadence's I2C controller's transfer_size
> >
> > Any help in reproducing the conditions would be appreciated
> >
> >
> > > register will roll over and generate invalid read transactions. Before
> > > this change, the ISR relied solely on the RXDV bit to determine when to
> > > write more data to the user's buffer. The invalid read data would cause
> > > overruns, smashing stacks and worse.
> > >
> > > This change stops the buffer writes to the requested boundary and
> > > reports the error. The controller will be reset so normal transactions
> > > may resume.
> > >
> > > Signed-off-by: Alex Williams <alex.williams@ni.com>
>
>
> One possible related errata is here:
> https://www.xilinx.com/support/answers/61664.html
>
> In our case, we only needed to hammer on i2c to reproduce in a few
> minutes, with a script like this:
> while true
>     do date
>     cat /sys/class/gpio/gpio882/direction > /dev/null
>     cat /sys/class/gpio/gpio883/direction > /dev/null
>     cat /sys/class/gpio/gpio884/direction > /dev/null
>     cat /sys/class/gpio/gpio885/direction > /dev/null
>     cat /sys/class/gpio/gpio886/direction > /dev/null
>     cat /sys/class/gpio/gpio887/direction > /dev/null
>     cat /sys/class/gpio/gpio888/direction > /dev/null
>     cat /sys/class/gpio/gpio889/direction > /dev/null
>     cat /sys/class/gpio/gpio890/direction > /dev/null
>     cat /sys/class/gpio/gpio891/direction > /dev/null
>     cat /sys/class/gpio/gpio892/direction > /dev/null
>
>     cat /sys/class/gpio/gpio894/direction > /dev/null
>     cat /sys/class/gpio/gpio895/direction > /dev/null
>     cat /sys/class/gpio/gpio896/direction > /dev/null
>     cat /sys/class/gpio/gpio897/direction > /dev/null
>     cat /sys/class/gpio/gpio898/direction > /dev/null
>     cat /sys/class/gpio/gpio899/direction > /dev/null
>     cat /sys/class/gpio/gpio900/direction > /dev/null
>     cat /sys/class/gpio/gpio901/direction > /dev/null
>     cat /sys/class/gpio/gpio902/direction > /dev/null
>     cat /sys/class/gpio/gpio903/direction > /dev/null
>     cat /sys/class/gpio/gpio904/direction > /dev/null
>     cat /sys/class/gpio/gpio905/direction > /dev/null
> done
>
> In normal usage, we have code that sets up a number of i2c GPIO
> expanders and pokes them for values as it initializes devices.
> Occasionally, the transfer size register will roll over, and the ISR
> will cause memory corruption, since it doesn't stop writing at the
> requested boundary.
Reviewed-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Wolfram Sang Jan. 30, 2020, 8:03 a.m. UTC | #3
On Thu, Jan 31, 2019 at 01:39:57PM -0800, alex.williams@ettus.com wrote:
> From: Alex Williams <alex.williams@ni.com>
> 
> Under certain conditions, Cadence's I2C controller's transfer_size
> register will roll over and generate invalid read transactions. Before
> this change, the ISR relied solely on the RXDV bit to determine when to
> write more data to the user's buffer. The invalid read data would cause
> overruns, smashing stacks and worse.
> 
> This change stops the buffer writes to the requested boundary and
> reports the error. The controller will be reset so normal transactions
> may resume.
> 
> Signed-off-by: Alex Williams <alex.williams@ni.com>

Applied to for-next with another Rev-by from Michal given in another
thread, thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index b13605718291..64e1d9e888c3 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -213,6 +213,7 @@  static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 
 	isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
 	cdns_i2c_writereg(isr_status, CDNS_I2C_ISR_OFFSET);
+	id->err_status = 0;
 
 	/* Handling nack and arbitration lost interrupt */
 	if (isr_status & (CDNS_I2C_IXR_NACK | CDNS_I2C_IXR_ARB_LOST)) {
@@ -246,10 +247,17 @@  static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 			    !id->bus_hold_flag)
 				cdns_i2c_clear_bus_hold(id);
 
-			*(id->p_recv_buf)++ =
-				cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);
-			id->recv_count--;
-			id->curr_recv_count--;
+			if (id->recv_count > 0) {
+				*(id->p_recv_buf)++ =
+					cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);
+				id->recv_count--;
+				id->curr_recv_count--;
+			} else {
+				dev_err(id->adap.dev.parent,
+					"xfer_size reg rollover. xfer aborted!\n");
+				id->err_status |= CDNS_I2C_IXR_TO;
+				break;
+			}
 
 			if (cdns_is_holdquirk(id, hold_quirk))
 				break;
@@ -347,7 +355,7 @@  static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 	}
 
 	/* Update the status for errors */
-	id->err_status = isr_status & CDNS_I2C_IXR_ERR_INTR_MASK;
+	id->err_status |= isr_status & CDNS_I2C_IXR_ERR_INTR_MASK;
 	if (id->err_status)
 		status = IRQ_HANDLED;