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]
Date:   Mon, 13 Jan 2020 09:06:46 +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

Hi, Michael,

On Sunday, January 12, 2020 1:16:12 AM EET Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> Hi Tudor,
> 
> Am 2020-01-11 15:19, schrieb Tudor.Ambarus@...rochip.com:
> > Hi, Michael,
> > 
> > On Saturday, January 4, 2020 12:34:23 AM EET Michael Walle wrote:
> >> Add support for the Winbond W25QnnJW-IM flashes. These have a
> >> programmable QE bit. There are also the W25QnnJW-IQ variant which
> >> shares
> >> the ID with the W25QnnFW parts. These have the QE bit hard strapped to
> >> 1, thus don't support hardware write protection.
> > 
> > There are few flavors of hw write protection supported by this flash,
> > the Q
> > version does not disable them all. How about saying just that the /HOLD
> > function is disabled?
> 
> I don't get your point here ;) My understanding is that HOLD# and WP#
> will
> be disabled. Thus there is no "hardware write protection". What other hw
> write protection do you have in mind?

Time delay write disable after Power-up for example.

> 
> > When we receive new flash id patches, we ask the contributors to
> > specify if
> > they test the flash, in which modes (single, quad), and with which
> > controller.
> > Ideally all the flash's flags should be tested, but there are cases in
> > which
> > the controllers do not support quad read for example, and we accept the
> > patches even if tested in single read mode. SPI_NOR_HAS_LOCK and
> > SPI_NOR_HAS_TB must be tested as well.
> > 
> > Even if the patches are rather simple, we ask for this to be sure that
> > we
> > don't add a flash that is broken from day one. So, would you please
> > tell us
> > what flashes did you test, what flags, and with which controller?
> 
> Ok will add that to the commit message. Just to make sure. I've only
> tested the
> 32mbit part. So is it still ok to include all other flashes of this
> family?

No, just the ones that you can test please.

> 
> For now. tested with the NXP FlexSPI, single and dual (no quad since we
> are
> using the write protection feature and IO2 and IO3 are not connected to
> the
> CPU). So write protection is also tested. I will retest the TB bit.

Great, thanks.

> 
> >> Signed-off-by: Michael Walle <michael@...le.cc>
> >> ---
> >> 
> >>  drivers/mtd/spi-nor/spi-nor.c | 22 ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >> 
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c
> >> b/drivers/mtd/spi-nor/spi-nor.c
> >> index addb6319fcbb..3fa8a81bdab0 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -2627,6 +2627,11 @@ static const struct flash_info spi_nor_ids[] =
> >> {
> >> 
> >>                      SECT_4K | SPI_NOR_DUAL_READ |
> > 
> > SPI_NOR_QUAD_READ |
> > 
> >>                      SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> >>      
> >>      },
> >> 
> >> +    {
> >> +            "w25q16jwim", INFO(0xef8015, 0, 64 * 1024,  32,
> > 
> > "i" is for the temperature range, which is not a fixed characteristic.
> > Usually
> > there are flashes with the same jedec-id, but with different
> > temperature
> > ranges. Let's drop the "i" and rename it to "w25q16jwm"
> 
> Only that there is no flash with that part name :( according to the
> datasheet

The datasheet describes the W25Q32JW flash (check the first page of the 
datasheet). There are two flavors of this flash, each with its own jedec-id: Q 
version uses 156016h, M 158016h. We should name this flashes as "w25q32jwq" 
and "w25q32jwm". Please notice that I skipped intentionally the "i"  that 
stands for temperature range. Manufacturers can provide better temperature 
ranges for the same flash without changing the jedec-id. See this datasheet:

https://ro.mouser.com/datasheet/2/949/w25q128jv_revf_03272018_plus-1489608.pdf

> there is only this one temp range available. From what I've seen for now
> (esp
> looking at the macronix parts) it seems to first come first serve ;)
> That being said, I don't insist on keeping that name, I'm fine with any
> name,

you should be fine just with the name that best describes the flash :)

> since I've learned you cannot rely on it in any way. Eg. the w25q32jwiq
> will
> be discovered as w25q32dw. Or some Macronix flashes will be discovered
> as
> ancient ones.

Would you please study what's wrong with these names and provide a patch to 
fix them?

> 
> 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?

Cheers,
ta

> better to just have the name "w25q16" regardless whether its an FW/JW/JV
> etc.
> It's better to show an ambiguous name than a wrong name.
> 
> -michael