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  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]
Date:   Sun, 24 Mar 2019 07:56:17 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Jan Kotas <jank@...ence.com>, LKML <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>, kernel@...a-handheld.com,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        linux-spi <linux-spi@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Brown <broonie@...nel.org>
Subject: Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

Hi Linus,

> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@...aro.org>:
> 
> On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@...delico.com> wrote:
> 
>> (1) c1c04cea13dc gpio: of: Fix logic inversion
>> 
>> together with the basic patch
>> 
>> (2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings
>> 
>> leads to a severe regression for our GTA04 board.
> 
> Sorry! :(
> 
> I found the same problem on my Nomadik board.
> 
> But I fixed it in that case by introducing a spi-cs-high into the DTS file:
> https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2

Yes, that of course works and is our temporary solution.

And I see that you also have it on the controller node and not the slave node.

> 
>> I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
>> from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
>> explains why it was not noticed earlier.
>> 
>> Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
>> in mind
> 
> I'm sorry about that, however if you look at the DT binding document:
> Documentation/devicetree/bindings/spi/spi-bus.txt

Shouldn't it be a property of the slave node and not the controller node?

> 
> You will see that spi-cs-high is mandatory. So these DTS files are
> incorrect.

How do you read that it is mandatory from

"All slave nodes can contain the following optional properties:
- spi-cs-high     - Empty property indicating device requires chip select
		    active high."

I read it as optional and indicative. Equal priority to cs-gpios.

> 
>> Therefore I would suggest:
>> * revert both patches as soon as possible (v5.1-rc series) to remove the unexpected spi legacy
>>  code handler from the gpio subsystem.
>> * replace all uses of spi-cs-high by correct cs-gpios flags - unless they already are there.
>>  fgrep spi-cs-high arch/*/boot/dts/*.dts* shows only a handful of DTS candidates.
>> * fix spi-bus.txt documentation to describe this potential pitfall.
> 
> This does not work because there are devices that requires spi-cs-high to be
> respected and the DTS second cell GPIO flag to be ignored.

Then, those should be fixed...

> 
> Jan Kotas reported this problem.

Thanks for adding him to the discussion.

> 
> They might have deployed DTB binaries that need to keep working,

Well, that is a weak argument. What if the GTA04 would have the DTB in FLASH
and would need it working (fortunately we always reflash kernel + dtbs)?

> so we
> cannot change it to ignore spi-cs-high like this. (I might give in if it can be
> proven that all of them just recompile the DTS all the time and no
> DTBs are in flash.)

BTW, on which node do these invariable DTBs have the spi-cs-high flag?
In the controller node (IMHO wrong) or the slave node (according to bindings doc)?

I have checked with randomly picked imx51-babbage.dts and it has it in the
slave node (pmic: mc13892@0). It also seems to be an example where different
CS lines want different settings.

If the all these DTBs have spi-cs-high in the slave node, none of them will
be fixed by the current code.

> 
> I think in this case the oldest binding wins.

Ok, it is the question when such deprecated things are really removed.
There is no clear answer...

> The spi-cs-high was there before
> we came up with the scheme to use the flags cell with GPIO phandles.

> 
> I think you simply have to patch these GTA04 DTS files to use
> spi-cs-high.

Ugly... Well, if DTS maintainers accept that?

> 
> But I'm open to other ideas, let's discuss this.

What about a CONFIG to explicitly enable/disable this legacy support?

IMHO it will need to be enabled for les than 1% of the kernel builds and
therefore also keeps the zImage smaller for all others. And avoids DTB
changes where the gpio flags are already correct.

BR and thanks,
Nikolaus

Powered by blists - more mailing lists