[PATCH RFC 1/3] i2c: bcm2835: Avoid possible NULL ptr dereference

[kernel at martin.sperl.org]

> On 21.02.2017, at 09:58, Stefan Wahren <stefan.wahren at 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 at 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 at tronnes.org>
>>>>> CC: Martin Sperl <kernel at martin.sperl.org>
>>>>> Reported-by: Peter Robinson <pbrobinson at gmail.com>
>>>>> Fixes: e2474541032d ("bcm2835: Fix hang for writing messages larger than 16 bytes")
>>>>> Signed-off-by: Stefan Wahren <stefan.wahren at 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 at 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 at 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