linux-kernel - Re: [PATCH 1/2] [i2c-bcm2835] Fully clean up hardware state machine after a timeout

Open Source and information security mailing list archives

Message-ID: <CAPY8ntBQ9MVYZVwndYsGNu+w8Jed727W4zCAZmVonSbRvsf32w@mail.gmail.com>
Date:   Tue, 31 Oct 2023 15:14:41 +0000
From:   Dave Stevenson <dave.stevenson@...pberrypi.com>
To:     Andi Shyti <andi.shyti@...nel.org>
Cc:     mike.isely@...altdigital.com,
        Florian Fainelli <florian.fainelli@...adcom.com>,
        Mike Isely <isely@...ox.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Ray Jui <rjui@...adcom.com>,
        Scott Branden <sbranden@...adcom.com>,
        linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] [i2c-bcm2835] Fully clean up hardware state machine
 after a timeout

Hi Mike & Andi

On Tue, 31 Oct 2023 at 11:44, Andi Shyti <andi.shyti@...nel.org> wrote:
>
> Hi Mike,
>
> > When the driver detects a timeout, there's no guarantee that the ISR
> > would have fired.  Thus after a timeout, it's the foreground that
> > becomes responsible to reset the hardware state machine.  The change
> > here just duplicates what is already implemented in the ISR.
>
> Is this a fix? What failing here?
>
> Can we have a feedback from Florian, Ray or Scott here?
>
> ...
>
> >       if (!time_left) {
> > +             /* Since we can't trust the ISR to have cleaned up, do the
> > +              * full cleanup here... */
>
> Please use the
>
>         /*
>          * comment
>          * comment
>          */
>
> format
>
> >               bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
> >                                  BCM2835_I2C_C_CLEAR);
> > +             bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, BCM2835_I2C_S_CLKT |
> > +                                BCM2835_I2C_S_ERR | BCM2835_I2C_S_DONE);
>
> I'm not sure this is really making any difference though. How
> have you tested this?
>
> Have you tried reading those registers before and understand what
> went wrong?

I guess we may have a race hazard here.
The completion has timed out. The write to I2C_C will flush the FIFOs
and mask the interrupts out, so if the transaction can complete at
that point then the ISR won't handle clearing the status flags. As the
flags are "write 1 to reset", they could still be set for the next
transfer.
It'd be down to the exact timing of hitting I2C_C_CLEAR (to clear the
FIFOs and disable the interrupts) and when that terminates the
transfer. Ensuring the status bits are cleared will do no harm, but
I'm not convinced that there is an issue in the first place that needs
fixing. Can you give any more detail of when you've seen this fail?

  Dave

> Andi
>
>
> _______________________________________________
> linux-rpi-kernel mailing list
> linux-rpi-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives