linux-kernel - Re: [PATCH 11/17] spi: dw: Fix native CS being unset

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>] [day] [month] [year] [list]

Message-ID: <CACRpkdY=wkgnYPcqSzyzNpS6ckJZs-9kXfTfdwa1E+POzOBQGA@mail.gmail.com>
Date:   Fri, 8 May 2020 21:39:02 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Gregory Clement <gregory.clement@...tlin.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Serge Semin <fancer.lancer@...il.com>,
        Georgy Vlasov <Georgy.Vlasov@...kalelectronics.ru>,
        Ramil Zaripov <Ramil.Zaripov@...kalelectronics.ru>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
        Paul Burton <paulburton@...nel.org>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Arnd Bergmann <arnd@...db.de>,
        Allison Randal <allison@...utok.net>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Gareth Williams <gareth.williams.jx@...esas.com>,
        Rob Herring <robh+dt@...nel.org>, linux-mips@...r.kernel.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        Phil Edworthy <phil.edworthy@...esas.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alexios Zavras <alexios.zavras@...el.com>,
        Thor Thayer <thor.thayer@...ux.intel.com>,
        "wuxu.wu" <wuxu.wu@...wei.com>,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 11/17] spi: dw: Fix native CS being unset

On Fri, May 8, 2020 at 3:31 PM Serge Semin
<Sergey.Semin@...kalelectronics.ru> wrote:

> Commit 6e0a32d6f376 ("spi: dw: Fix default polarity of native
> chipselect") attempted to fix the problem when GPIO active-high
> chip-select is utilized to communicate with some SPI slave. It fixed
> the problem, but broke the normal native CS support. At the same time
> the reversion commit ada9e3fcc175 ("spi: dw: Correct handling of native
> chipselect") didn't solve the problem either, since it just inverted
> the set_cs() polarity perception without taking into account that
> CS-high might be applicable. Here is what is done to finally fix the
> problem.

I'm not sure this is the whole story.

I think Charles' fix made it work, and then commit
3e5ec1db8bfee845d9f8560d1c64aeaccd586398
"spi: Fix SPI_CS_HIGH setting when using native and GPIO CS"
fixed it broken again.

This commit will make sure only set SPI_CS_HIGH on a
spi_device if it is using a GPIO as CS. Before this change,
the core would set that on everything, and expect the
.set_cs() callback to cope.

I think we fixed that and that fix should have been undone
when applying commit 3e5ec1db8bfe.

So possibly Fixes: should be set only to this commit, so
that the fix is not backported to kernels without it.

> DW SPI controller demands any native CS being set in order to proceed
> with data transfer. So in order to activate the SPI communications we
> must set any bit in the Slave Select DW SPI controller register no
> matter whether the platform requests the GPIO- or native CS.

Ah-ha! Maybe we should even add a comment explaining that.
And that is why SPI_MASTER_GPIO_SS is set.

I suppose my naive understanding was:
"bit set to 1" = CS asserted (driven low)
"bit set to 0" = CS de-asserted  (driven high)
So that is not how this register works at all.

> This commit fixes the problem for all described cases. So no matter
> whether an SPI slave needs GPIO- or native-based CS with active-high
> or low signal the corresponding bit will be set in SER.

Makes sense.

>         struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
>         struct chip_data *chip = spi_get_ctldata(spi);
> +       bool cs_high = !!(spi->mode & SPI_CS_HIGH);
>
>         /* Chip select logic is inverted from spi_set_cs() */
>         if (chip && chip->cs_control)
>                 chip->cs_control(!enable);
>
> -       if (!enable)
> +       if (cs_high == enable)
>                 dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));

This is the correct fix now but I an afraid not correct before
commit 3e5ec1db8bfe.

What I can't help but asking is: can the native chip select even
handle active high chip select if not backed by a GPIO?
Which register would set that polarity?

Yours,
Linus Walleij