linux-kernel - Re: [PATCH RFC v1] spi: realtek-rtl: Fix clearing some register bits

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFBinCDZYb-Pt_Vn14cDmprM6CxRoEbTUTx16FMv=cWf-pnrQw@mail.gmail.com>
Date:   Wed, 27 Jul 2022 21:13:02 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Sander Vanheule <sander@...nheule.net>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org,
        bert@...t.com, mail@...ger-koblitz.de
Subject: Re: [PATCH RFC v1] spi: realtek-rtl: Fix clearing some register bits

Hi Sander,

On Tue, Jul 26, 2022 at 11:03 AM Sander Vanheule <sander@...nheule.net> wrote:
[...]
> >         value = readl(REG(RTL_SPI_SFCSR));
> > -       value &= RTL_SPI_SFCSR_LEN_MASK;
> > +       value &= ~RTL_SPI_SFCSR_LEN_MASK;
>
> Although typically a field mask has the only the bits of that field set,
> RTL_SPI_SFCSR_LEN_MASK is already inverted. So LEN_MASK has all bits set,
> *except* for those where LEN is stored.
>
> This means the code currently is:
>         value &= ~(0x3 << 28);
>
> which is correct AFAICT, as it clears the LEN bits, but keeps all the others.
Thank you for this hint! I completely missed that when reading the
definition of the macro.

> While this part is currently not wrong, I wouldn't be opposed to a patch to make
> it less confusing by not inverting the field mask in the definition of
> RTL_SPI_SFCSR_LEN_MASK.
I can re-spin this patch and move the ~ operator where most people
expect it to be.

> >         if (size == 4)
> >                 value |= RTL_SPI_SFCSR_LEN4;
> >         else if (size == 1)
> > @@ -143,7 +143,7 @@ static void init_hw(struct rtspi *rtspi)
> >         /* Permanently disable CS1, since it's never used */
> >         value |= RTL_SPI_SFCSR_CSB1;
> >         /* Select CS0 for use */
> > -       value &= RTL_SPI_SFCSR_CS;
> > +       value &= ~RTL_SPI_SFCSR_CS;
>
> This macro is not inverted, so it does clear any previously set bits, and
> probably doesn't end up with RTL_SPI_SFCRS_CS set. However, is in an init call
> and it doesn't appear to cause any issues later on, right? Is this because the
> SFCSR register is (unintentionally) cleared and that is actually required?
I'm not sure what's right or wrong here but the code reads strange:
    value = readl(...);
    value |= BIT(30); /* RTL_SPI_SFCSR_CSB1 */
    value &= BIT(24); /* RTL_SPI_SFCSR_CS */
What's the point in setting RTL_SPI_SFCSR_CSB1 (bit 30) when it's
immediately cleared in the next operation?

Also any bits read from the register except RTL_SPI_SFCSR_CS (bit 24)
are cleared - why even bother reading that register then?

If you have any advice on how to change this code then I'm happy to do so.
Otherwise I'd leave it as is, especially since I cannot test this in any way.


Best regards,
Martin