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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > CC: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> > > > Reported-by: Peter Robinson <pbrobinson@xxxxxxxxx>
> > > > Fixes: e2474541032d ("bcm2835: Fix hang for writing messages
> > > > larger than 16 bytes")
> > > > Signed-off-by: Stefan Wahren <stefan.wahren@xxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxx>
> > >
> >
> > 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
