linux-kernel - Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

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: <3862353.UOg0IvECEa@localhost.localdomain>
Date:   Mon, 20 Jan 2020 11:03:21 +0000
From:   <Tudor.Ambarus@...rochip.com>
To:     <michael@...le.cc>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <richard@....at>, <vigneshr@...com>, <miquel.raynal@...tlin.com>
Subject: Re: [PATCH] mtd: spi-nor: Add support for w25qNNjwim

On Monday, January 20, 2020 12:24:25 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,

Hi, Michael,

> 
> >> Am 2020-01-13 11:07, schrieb Michael Walle:
> >> >>> Btw. is renaming the flashes also considered a backwards incomaptible
> >> >>> change?
> >> >> 
> >> >> No, we can fix the names.
> >> >> 
> >> >>> And can there be two flashes with the same name? Because IMHO it
> >> >>> would
> >> >>> be
> >> >> 
> >> >> I would prefer that we don't. Why would you have two different
> >> >> jedec-ids with
> >> >> the same name?
> >> > 
> >> > Because as pointed out in the Winbond example you cannot distiguish
> >> > between
> >> > W25Q32DW and W25Q32JWIQ; and in the Macronix example between MX25L8005
> >> > and
> >> > MX25L8006E. Thus my reasoning was to show only the common part, ie
> >> > W25Q32
> >> > or MX25L80 which should be the same for this particular ID. Like I
> >> > said, I'd
> >> > prefer showing an ambiguous name instead of a wrong one. But then you
> >> > may
> >> > have different IDs with the same ambiguous name.
> >> 
> >> Another solution would be to have the device tree provide a hint for
> >> the
> >> actual flash chip. There would be multiple entries in the spi_nor_ids
> >> with the
> >> same flash id. By default the first one is used (keeping the current
> >> behaviour). If there is for example
> >> 
> >>    compatible = "jedec,spi-nor", "w25q32jwq";
> >> 
> >> the flash_info for the w25q32jwq will be chosen.
> > 
> > This won't work for plug-able flashes. You will influence the name in
> > dt to be
> > chosen as w25q32jwq, and if you change w25q32jwq with w25q32dw you will
> > end up
> > with a wrong name for w25q32dw, thus the same problem.
> 
> No, because then the device tree is wrong and doesn't fit the hardware.
> You'd
> have to some instance which could change the device tree node, like the
> bootloader or some device tree overlay for plugable flashes. We should
> try to
> solve the actual problem at hand first..
> 
> It is just not possible to autodetect the SPI flash, just because
> the vendors reuse the same IDs for flashes with different features (and
> the
> SFDP is likely not enough). Therefore, you need to have a hint in some
> place
> to use the flash properly.
> 
> > If the flashes are identical but differ just in terms of name, we can
> > rename
> > the flash to "w25q32jwq (w25q32dw)". I haven't studied the differences
> > between
> > these flashes; if you want to fix them, send a patch and I'll try to
> > help.
> 
> It is not only the name, here are two examples which differ in
> functionality:
>   (1) mx25l8005 doesn't support dual/quad mode. mx25l8006e supports
> dual/quad
>       mode
>   (2) mx25u3235f doesn't support TB bit, mx25u3232e has a TB bit.
> 
> well.. to repeat myself, the mx25l25635_post_bfpt_fixups is a third

sorry if this exhausted you.
> example.
> 

Flash auto-detection is nice and we should preserve it if possible. I would 
prefer having a post bfpt fixup than giving a hint about the flash in the 
compatible. The flashes that you mention are quite old and I don't know if it 
is worth to harm the auto-detection for them. A compromise has to be made.

You can gain traction in your endeavor if you have such a flash and there's 
nothing auto-detectable that differentiates it from some other flash that 
shares the sama jedec-id.

If you have such a flash and you care about it, send a patch and I'll try to 
help.

> -michael
> 
> > Cheers,
> > ta
> > 
> >> I know this will conflict with the new rule that there should only be
> >> 
> >>    compatible = "jedec,spi-nor";
> >> 
> >> without the actual flash chip. But it seems that it is not always
> >> possible
> >> to just use the jedec id to match the correct chip.
> >> 
> >> Also see for example mx25l25635_post_bfpt_fixups() which tries to
> >> figure
> >> out different behaviour by looking at "some" SFDP data. In this case
> >> we
> >> might have been lucky, but I fear that this won't work in all cases
> >> and
> >> for older flashes it won't work at all.
> >> 
> >> BTW I do not suggest to add the strings to the the spi_nor_dev_ids[].
> >> 
> >> I guess that would be a less invasive way to fix different flashes
> >> with
> >> same jedec ids.
> >> 
> >> -michael