| Message ID | 1487280047-29608-2-git-send-email-stefan.wahren@i2se.com |
|---|---|
| State | Accepted |
| Headers | show |
Den 16.02.2017 22.20, skrev Stefan Wahren: > Since commit e2474541032d ("bcm2835: Fix hang for writing messages > larger than 16 bytes") the interrupt handler is prone to a possible > NULL pointer dereference. This could happen if an interrupt fires > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs > on the RPi 3. Even this is an unexpected behavior the driver must > handle that with an error instead of a crash. > > CC: Noralf Trønnes <noralf@tronnes.org> > CC: Martin Sperl <kernel@martin.sperl.org> > Reported-by: Peter Robinson <pbrobinson@gmail.com> > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > --- > drivers/i2c/busses/i2c-bcm2835.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > index c3436f6..10e39c8 100644 > --- a/drivers/i2c/busses/i2c-bcm2835.c > +++ b/drivers/i2c/busses/i2c-bcm2835.c > @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) > } > > if (val & BCM2835_I2C_S_DONE) { > - if (i2c_dev->curr_msg->flags & I2C_M_RD) { > + if (!i2c_dev->curr_msg) { > + dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n"); > + } else if (i2c_dev->curr_msg->flags & I2C_M_RD) { > bcm2835_drain_rxfifo(i2c_dev); > val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); > } Thanks, Acked-by: Noralf Trønnes <noralf@tronnes.org> -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote: > Since commit e2474541032d ("bcm2835: Fix hang for writing messages > larger than 16 bytes") the interrupt handler is prone to a possible > NULL pointer dereference. This could happen if an interrupt fires > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs > on the RPi 3. Even this is an unexpected behavior the driver must > handle that with an error instead of a crash. > > CC: Noralf Trønnes <noralf@tronnes.org> > CC: Martin Sperl <kernel@martin.sperl.org> > Reported-by: Peter Robinson <pbrobinson@gmail.com> > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> Applied to for-next, thanks (will be in 4.11)! Note for patches 2+3: I usually don't take DTS changes via I2C, so this likely needs to go via arm-soc or some other bcm tree.
Hi, > Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um 19:34 geschrieben: > > > > Den 16.02.2017 22.20, skrev Stefan Wahren: > > Since commit e2474541032d ("bcm2835: Fix hang for writing messages > > larger than 16 bytes") the interrupt handler is prone to a possible > > NULL pointer dereference. This could happen if an interrupt fires > > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs > > on the RPi 3. Even this is an unexpected behavior the driver must > > handle that with an error instead of a crash. > > > > CC: Noralf Trønnes <noralf@tronnes.org> > > CC: Martin Sperl <kernel@martin.sperl.org> > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > > --- > > drivers/i2c/busses/i2c-bcm2835.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c > > index c3436f6..10e39c8 100644 > > --- a/drivers/i2c/busses/i2c-bcm2835.c > > +++ b/drivers/i2c/busses/i2c-bcm2835.c > > @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) > > } > > > > if (val & BCM2835_I2C_S_DONE) { > > - if (i2c_dev->curr_msg->flags & I2C_M_RD) { > > + if (!i2c_dev->curr_msg) { > > + dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n"); > > + } else if (i2c_dev->curr_msg->flags & I2C_M_RD) { > > bcm2835_drain_rxfifo(i2c_dev); > > val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); > > } > > Thanks, > > Acked-by: Noralf Trønnes <noralf@tronnes.org> > thanks, but i would be more happier to receive feedback for patches 2 and 3. Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Den 20.02.2017 20.40, skrev Stefan Wahren: > Hi, > >> Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um 19:34 geschrieben: >> >> >> >> Den 16.02.2017 22.20, skrev Stefan Wahren: >>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages >>> larger than 16 bytes") the interrupt handler is prone to a possible >>> NULL pointer dereference. This could happen if an interrupt fires >>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs >>> on the RPi 3. Even this is an unexpected behavior the driver must >>> handle that with an error instead of a crash. >>> >>> CC: Noralf Trønnes <noralf@tronnes.org> >>> CC: Martin Sperl <kernel@martin.sperl.org> >>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") >>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >>> --- >>> drivers/i2c/busses/i2c-bcm2835.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c >>> index c3436f6..10e39c8 100644 >>> --- a/drivers/i2c/busses/i2c-bcm2835.c >>> +++ b/drivers/i2c/busses/i2c-bcm2835.c >>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) >>> } >>> >>> if (val & BCM2835_I2C_S_DONE) { >>> - if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>> + if (!i2c_dev->curr_msg) { >>> + dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n"); >>> + } else if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>> bcm2835_drain_rxfifo(i2c_dev); >>> val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); >>> } >> Thanks, >> >> Acked-by: Noralf Trønnes <noralf@tronnes.org> >> > thanks, but i would be more happier to receive feedback for patches 2 and 3. I have only worked on dts files downstream and never done any arm64 stuff, so I'm not up to speed on this. Noralf. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 20.02.2017 um 22:22 schrieb Noralf Trønnes: > > Den 20.02.2017 20.40, skrev Stefan Wahren: >> Hi, >> >>> Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um 19:34 >>> geschrieben: >>> >>> >>> >>> Den 16.02.2017 22.20, skrev Stefan Wahren: >>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages >>>> larger than 16 bytes") the interrupt handler is prone to a possible >>>> NULL pointer dereference. This could happen if an interrupt fires >>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs >>>> on the RPi 3. Even this is an unexpected behavior the driver must >>>> handle that with an error instead of a crash. >>>> >>>> CC: Noralf Trønnes <noralf@tronnes.org> >>>> CC: Martin Sperl <kernel@martin.sperl.org> >>>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger >>>> than 16 bytes") >>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >>>> --- >>>> drivers/i2c/busses/i2c-bcm2835.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c >>>> b/drivers/i2c/busses/i2c-bcm2835.c >>>> index c3436f6..10e39c8 100644 >>>> --- a/drivers/i2c/busses/i2c-bcm2835.c >>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c >>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int >>>> this_irq, void *data) >>>> } >>>> if (val & BCM2835_I2C_S_DONE) { >>>> - if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>>> + if (!i2c_dev->curr_msg) { >>>> + dev_err(i2c_dev->dev, "Got unexpected interrupt (from >>>> firmware?)\n"); >>>> + } else if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>>> bcm2835_drain_rxfifo(i2c_dev); >>>> val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); >>>> } >>> Thanks, >>> >>> Acked-by: Noralf Trønnes <noralf@tronnes.org> >>> >> thanks, but i would be more happier to receive feedback for patches 2 >> and 3. > > I have only worked on dts files downstream and never done any arm64 > stuff, so I'm not up to speed on this. > > Noralf. > Okay, at the end i only want to know 2 things: Does new message "Got unexpected interrupt" still appear after applying all 3 patches? Does the annoying message "i2c-bcm2835 3f805000.i2c: i2c transfer timed out" still appear after applying all 3 patches? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 21.02.2017, at 09:58, Stefan Wahren <stefan.wahren@i2se.com> wrote: > > Am 20.02.2017 um 22:22 schrieb Noralf Trønnes: >> >> Den 20.02.2017 20.40, skrev Stefan Wahren: >>> Hi, >>> >>>> Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um 19:34 geschrieben: >>>> >>>> >>>> >>>> Den 16.02.2017 22.20, skrev Stefan Wahren: >>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages >>>>> larger than 16 bytes") the interrupt handler is prone to a possible >>>>> NULL pointer dereference. This could happen if an interrupt fires >>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs >>>>> on the RPi 3. Even this is an unexpected behavior the driver must >>>>> handle that with an error instead of a crash. >>>>> >>>>> CC: Noralf Trønnes <noralf@tronnes.org> >>>>> CC: Martin Sperl <kernel@martin.sperl.org> >>>>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") >>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >>>>> --- >>>>> drivers/i2c/busses/i2c-bcm2835.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c >>>>> index c3436f6..10e39c8 100644 >>>>> --- a/drivers/i2c/busses/i2c-bcm2835.c >>>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c >>>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) >>>>> } >>>>> if (val & BCM2835_I2C_S_DONE) { >>>>> - if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>>>> + if (!i2c_dev->curr_msg) { >>>>> + dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n"); >>>>> + } else if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>>>> bcm2835_drain_rxfifo(i2c_dev); >>>>> val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); >>>>> } >>>> Thanks, >>>> >>>> Acked-by: Noralf Trønnes <noralf@tronnes.org> >>>> >>> thanks, but i would be more happier to receive feedback for patches 2 and 3. >> >> I have only worked on dts files downstream and never done any arm64 >> stuff, so I'm not up to speed on this. >> >> Noralf. >> > > Okay, at the end i only want to know 2 things: > > Does new message "Got unexpected interrupt" still appear after applying all 3 patches? > > Does the annoying message "i2c-bcm2835 3f805000.i2c: i2c transfer timed out" still appear after applying all 3 patches? I believe the issue is primarily that i2c0@3f805000 is “owned” by the firmware. This is less of a problem on the rpi1 and rpi2, as there is little to no activity on those on i2c0 by the firmware (besides when using the camera). But on the rpi3 some Voltage sensors as well as GPIO extenders are attached to this bus, which are frequently read/modified by the firmware. So in principle this i2c bus should be disabled. But if it is then there will be an interrupt triggered on the VC4 core as well as on the ARM core - both will see the interrupt. but typically the RT-firmware will respond much faster and disable the irq sources already before the arm gets to “detect” the active interrupt in the irq handler. But sometimes the firmware is not fast enough (or the irq path is currently active in linux at the right time) so the interrupt handler will trigger even if there was no running i2c transfer on that bus. The patch is basically defensive programming to avoid such cases. Martin -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-02-20 at 22:22 +0100, Noralf Trønnes wrote: > Den 20.02.2017 20.40, skrev Stefan Wahren: > > Hi, > > > > > Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um > > > 19:34 geschrieben: > > > > > > > > > > > > Den 16.02.2017 22.20, skrev Stefan Wahren: > > > > Since commit e2474541032d ("bcm2835: Fix hang for writing > > > > messages > > > > larger than 16 bytes") the interrupt handler is prone to a > > > > possible > > > > NULL pointer dereference. This could happen if an interrupt > > > > fires > > > > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly > > > > occurs > > > > on the RPi 3. Even this is an unexpected behavior the driver > > > > must > > > > handle that with an error instead of a crash. > > > > > > > > CC: Noralf Trønnes <noralf@tronnes.org> > > > > CC: Martin Sperl <kernel@martin.sperl.org> > > > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > > > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages > > > > larger than 16 bytes") > > > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > --- > > > > drivers/i2c/busses/i2c-bcm2835.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/i2c/busses/i2c-bcm2835.c > > > > b/drivers/i2c/busses/i2c-bcm2835.c > > > > index c3436f6..10e39c8 100644 > > > > --- a/drivers/i2c/busses/i2c-bcm2835.c > > > > +++ b/drivers/i2c/busses/i2c-bcm2835.c > > > > @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int > > > > this_irq, void *data) > > > > } > > > > > > > > if (val & BCM2835_I2C_S_DONE) { > > > > - if (i2c_dev->curr_msg->flags & I2C_M_RD) { > > > > + if (!i2c_dev->curr_msg) { > > > > + dev_err(i2c_dev->dev, "Got unexpected > > > > interrupt (from firmware?)\n"); > > > > + } else if (i2c_dev->curr_msg->flags & > > > > I2C_M_RD) { > > > > bcm2835_drain_rxfifo(i2c_dev); > > > > val = bcm2835_i2c_readl(i2c_dev, > > > > BCM2835_I2C_S); > > > > } > > > > > > Thanks, > > > > > > Acked-by: Noralf Trønnes <noralf@tronnes.org> > > > > > > > thanks, but i would be more happier to receive feedback for patches > > 2 and 3. > > I have only worked on dts files downstream and never done any arm64 > stuff, so I'm not up to speed on this. > > Noralf. > Just a note, the original downstream driver returns IRQ_NONE in this case. Does that make any difference? Also, the the original driver has the check at the start of the IRQ and it doesn't log an error message. I'm wondering if logging at err level is the best since it might make people think a serious error has happened. I think by default this would show up on the console, correct? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 21.02.2017 um 16:07 schrieb Michael Zoran: > On Mon, 2017-02-20 at 22:22 +0100, Noralf Trønnes wrote: >> Den 20.02.2017 20.40, skrev Stefan Wahren: >>> Hi, >>> >>>> Noralf Trønnes <noralf@tronnes.org> hat am 18. Februar 2017 um >>>> 19:34 geschrieben: >>>> >>>> >>>> >>>> Den 16.02.2017 22.20, skrev Stefan Wahren: >>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing >>>>> messages >>>>> larger than 16 bytes") the interrupt handler is prone to a >>>>> possible >>>>> NULL pointer dereference. This could happen if an interrupt >>>>> fires >>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly >>>>> occurs >>>>> on the RPi 3. Even this is an unexpected behavior the driver >>>>> must >>>>> handle that with an error instead of a crash. >>>>> >>>>> CC: Noralf Trønnes <noralf@tronnes.org> >>>>> CC: Martin Sperl <kernel@martin.sperl.org> >>>>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages >>>>> larger than 16 bytes") >>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >>>>> --- >>>>> drivers/i2c/busses/i2c-bcm2835.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/i2c/busses/i2c-bcm2835.c >>>>> b/drivers/i2c/busses/i2c-bcm2835.c >>>>> index c3436f6..10e39c8 100644 >>>>> --- a/drivers/i2c/busses/i2c-bcm2835.c >>>>> +++ b/drivers/i2c/busses/i2c-bcm2835.c >>>>> @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int >>>>> this_irq, void *data) >>>>> } >>>>> >>>>> if (val & BCM2835_I2C_S_DONE) { >>>>> - if (i2c_dev->curr_msg->flags & I2C_M_RD) { >>>>> + if (!i2c_dev->curr_msg) { >>>>> + dev_err(i2c_dev->dev, "Got unexpected >>>>> interrupt (from firmware?)\n"); >>>>> + } else if (i2c_dev->curr_msg->flags & >>>>> I2C_M_RD) { >>>>> bcm2835_drain_rxfifo(i2c_dev); >>>>> val = bcm2835_i2c_readl(i2c_dev, >>>>> BCM2835_I2C_S); >>>>> } >>>> Thanks, >>>> >>>> Acked-by: Noralf Trønnes <noralf@tronnes.org> >>>> >>> thanks, but i would be more happier to receive feedback for patches >>> 2 and 3. >> I have only worked on dts files downstream and never done any arm64 >> stuff, so I'm not up to speed on this. >> >> Noralf. >> > Just a note, the original downstream driver returns IRQ_NONE in this > case. Does that make any difference? Yes, i decided to handle the IRQ in order to avoid a possible interrupt storm. Please look at the github discussion [1] for more details. > > Also, the the original driver has the check at the start of the IRQ and > it doesn't log an error message. > > I'm wondering if logging at err level is the best since it might make > people think a serious error has happened. It is a serious error and should be fixed by patch 2, 3 and a possible patch 4 later to disable i2c0. But before i disable i2c0 we need the test results of patch 2 and 3. The reason why i don't disable i2c at first is that Gerd [2] reported that disabling the PWM fixed the i2c timeout issue, which doesn't fit to Martins theory. [1] - https://github.com/anholt/linux/issues/89 [2] - http://lists.infradead.org/pipermail/linux-rpi-kernel/2016-June/003969.html -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, hi Wolfram, Am 20.02.2017 um 19:22 schrieb Wolfram Sang: > On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote: >> Since commit e2474541032d ("bcm2835: Fix hang for writing messages >> larger than 16 bytes") the interrupt handler is prone to a possible >> NULL pointer dereference. This could happen if an interrupt fires >> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs >> on the RPi 3. Even this is an unexpected behavior the driver must >> handle that with an error instead of a crash. >> >> CC: Noralf Trønnes <noralf@tronnes.org> >> CC: Martin Sperl <kernel@martin.sperl.org> >> Reported-by: Peter Robinson <pbrobinson@gmail.com> >> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") >> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > Applied to for-next, thanks (will be in 4.11)! > since this patch is too late for 4.10, should i resent with CC to stable in order to get it into the next 4.10 release? Stefan -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote: > >>Since commit e2474541032d ("bcm2835: Fix hang for writing messages > >>larger than 16 bytes") the interrupt handler is prone to a possible > >>NULL pointer dereference. This could happen if an interrupt fires > >>before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs > >>on the RPi 3. Even this is an unexpected behavior the driver must > >>handle that with an error instead of a crash. > >> > >>CC: Noralf Trønnes <noralf@tronnes.org> > >>CC: Martin Sperl <kernel@martin.sperl.org> > >>Reported-by: Peter Robinson <pbrobinson@gmail.com> > >>Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") > >>Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > >Applied to for-next, thanks (will be in 4.11)! > > > > since this patch is too late for 4.10, should i resent with CC to stable in > order to get it into the next 4.10 release? It has the Fixes: tag, that will do.
On Tue, Feb 21, 2017 at 09:14:03PM +0100, Wolfram Sang wrote: > > > >On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote: > > >>Since commit e2474541032d ("bcm2835: Fix hang for writing messages > > >>larger than 16 bytes") the interrupt handler is prone to a possible > > >>NULL pointer dereference. This could happen if an interrupt fires > > >>before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs > > >>on the RPi 3. Even this is an unexpected behavior the driver must > > >>handle that with an error instead of a crash. > > >> > > >>CC: Noralf Trønnes <noralf@tronnes.org> > > >>CC: Martin Sperl <kernel@martin.sperl.org> > > >>Reported-by: Peter Robinson <pbrobinson@gmail.com> > > >>Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") > > >>Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > > >Applied to for-next, thanks (will be in 4.11)! > > > > > > > since this patch is too late for 4.10, should i resent with CC to stable in > > order to get it into the next 4.10 release? > > It has the Fixes: tag, that will do. But it moves it much lower on my "this needs to get into stable now!" priority list. I'll try to remember this one when it goes by... thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, Am 22.02.2017 um 08:20 schrieb Greg Kroah-Hartman: > On Tue, Feb 21, 2017 at 09:14:03PM +0100, Wolfram Sang wrote: >>>> On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote: >>>>> Since commit e2474541032d ("bcm2835: Fix hang for writing messages >>>>> larger than 16 bytes") the interrupt handler is prone to a possible >>>>> NULL pointer dereference. This could happen if an interrupt fires >>>>> before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs >>>>> on the RPi 3. Even this is an unexpected behavior the driver must >>>>> handle that with an error instead of a crash. >>>>> >>>>> CC: Noralf Trønnes <noralf@tronnes.org> >>>>> CC: Martin Sperl <kernel@martin.sperl.org> >>>>> Reported-by: Peter Robinson <pbrobinson@gmail.com> >>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") >>>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> >>>> Applied to for-next, thanks (will be in 4.11)! >>>> >>> since this patch is too late for 4.10, should i resent with CC to stable in >>> order to get it into the next 4.10 release? >> It has the Fixes: tag, that will do. > But it moves it much lower on my "this needs to get into stable now!" > priority list. I'll try to remember this one when it goes by... > > thanks, > > greg k-h should i resend, since i didn't send you the initial patch? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 28, 2017 at 01:42:42PM +0100, Stefan Wahren wrote: > Hi Greg, > > Am 22.02.2017 um 08:20 schrieb Greg Kroah-Hartman: > > On Tue, Feb 21, 2017 at 09:14:03PM +0100, Wolfram Sang wrote: > > > > > On Thu, Feb 16, 2017 at 09:20:45PM +0000, Stefan Wahren wrote: > > > > > > Since commit e2474541032d ("bcm2835: Fix hang for writing messages > > > > > > larger than 16 bytes") the interrupt handler is prone to a possible > > > > > > NULL pointer dereference. This could happen if an interrupt fires > > > > > > before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs > > > > > > on the RPi 3. Even this is an unexpected behavior the driver must > > > > > > handle that with an error instead of a crash. > > > > > > > > > > > > CC: Noralf Trønnes <noralf@tronnes.org> > > > > > > CC: Martin Sperl <kernel@martin.sperl.org> > > > > > > Reported-by: Peter Robinson <pbrobinson@gmail.com> > > > > > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") > > > > > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> > > > > > Applied to for-next, thanks (will be in 4.11)! > > > > > > > > > since this patch is too late for 4.10, should i resent with CC to stable in > > > > order to get it into the next 4.10 release? > > > It has the Fixes: tag, that will do. > > But it moves it much lower on my "this needs to get into stable now!" > > priority list. I'll try to remember this one when it goes by... > > > > thanks, > > > > greg k-h > > should i resend, since i didn't send you the initial patch? Sure! -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c index c3436f6..10e39c8 100644 --- a/drivers/i2c/busses/i2c-bcm2835.c +++ b/drivers/i2c/busses/i2c-bcm2835.c @@ -195,7 +195,9 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data) } if (val & BCM2835_I2C_S_DONE) { - if (i2c_dev->curr_msg->flags & I2C_M_RD) { + if (!i2c_dev->curr_msg) { + dev_err(i2c_dev->dev, "Got unexpected interrupt (from firmware?)\n"); + } else if (i2c_dev->curr_msg->flags & I2C_M_RD) { bcm2835_drain_rxfifo(i2c_dev); val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S); }
Since commit e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") the interrupt handler is prone to a possible NULL pointer dereference. This could happen if an interrupt fires before curr_msg is set by bcm2835_i2c_xfer_msg() and randomly occurs on the RPi 3. Even this is an unexpected behavior the driver must handle that with an error instead of a crash. CC: Noralf Trønnes <noralf@tronnes.org> CC: Martin Sperl <kernel@martin.sperl.org> Reported-by: Peter Robinson <pbrobinson@gmail.com> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes") Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com> --- drivers/i2c/busses/i2c-bcm2835.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)