linux-kernel - Re: [PATCH v2 13/41] mtd: spi-nor: move the .id and .id

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: <c085d93b-2759-47ac-b21b-aac0b924349b@linaro.org>
Date:   Wed, 6 Sep 2023 08:15:11 +0100
From:   Tudor Ambarus <tudor.ambarus@...aro.org>
To:     Michael Walle <mwalle@...nel.org>
Cc:     Pratyush Yadav <pratyush@...nel.org>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        linux-kernel@...r.kernel.org, linux-mtd@...ts.infradead.org
Subject: Re: [PATCH v2 13/41] mtd: spi-nor: move the .id and .id_len into an
 own structure



On 9/6/23 08:13, Michael Walle wrote:
> Am 2023-09-06 08:12, schrieb Tudor Ambarus:
>> On 8/22/23 08:09, Michael Walle wrote:
>>> Create a new structure to hold a flash ID and its length. The goal is to
>>> have a new macro SNOR_ID() which can have a flexible id length. This way
>>> we can get rid of all the individual INFOx() macros.
>>>
>>> Signed-off-by: Michael Walle <mwalle@...nel.org>
>>> ---
>>>  drivers/mtd/spi-nor/core.c      |  6 +++---
>>>  drivers/mtd/spi-nor/core.h      | 33 ++++++++++++++++++++++++---------
>>>  drivers/mtd/spi-nor/micron-st.c |  4 ++--
>>>  drivers/mtd/spi-nor/spansion.c  |  4 ++--
>>>  drivers/mtd/spi-nor/sysfs.c     |  6 +++---
>>>  drivers/mtd/spi-nor/winbond.c   |  1 -
>>>  6 files changed, 34 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 4ba1778eda4b..80c340c7863a 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2028,8 +2028,8 @@ static const struct flash_info
>>> *spi_nor_match_id(struct spi_nor *nor,
>>>      for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
>>>          for (j = 0; j < manufacturers[i]->nparts; j++) {
>>>              part = &manufacturers[i]->parts[j];
>>> -            if (part->id_len &&
>>> -                !memcmp(part->id, id, part->id_len)) {
>>> +            if (part->id &&
>>> +                !memcmp(part->id->bytes, id, part->id->len)) {
>>>                  nor->manufacturer = manufacturers[i];
>>>                  return part;
>>>              }
>>> @@ -3370,7 +3370,7 @@ static const struct flash_info
>>> *spi_nor_get_flash_info(struct spi_nor *nor,
>>>       * If caller has specified name of flash model that can normally be
>>>       * detected using JEDEC, let's verify it.
>>>       */
>>> -    if (name && info->id_len) {
>>> +    if (name && info->id) {
>>>          const struct flash_info *jinfo;
>>>
>>>          jinfo = spi_nor_detect(nor);
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index c42b65623da7..81535f31907f 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -446,12 +446,22 @@ struct spi_nor_fixups {
>>>      int (*late_init)(struct spi_nor *nor);
>>>  };
>>>
>>> +/**
>>> + * struct spi_nor_id - SPI NOR flash ID.
>>> + *
>>> + * @bytes: the flash's ID bytes. The first three bytes are the JEDIC
>>> ID.
>>
>> typo, JEDIC. But are there 3 bytes of ID specified by JEDEC? I remeber
>> there's only the MFR ID specified, the rest are flash specific, which
>> are not mentioned by JEDEC.
> 
> I just moved the description from flash_info :p
> 
> As far as I know, this isn't really specified at all, is it? I couldn't
> even find that the "read jedec id" command 9F is specified anywhere. Do
> you know more?

it's not part of jesd215 as far as I remember, indeed.
> 
> Also the first three bytes is also wrong. It might be more or less. I'd
> treat it as opaque - the SNOR_ID() can now have an arbitrary length -
> but I would mention that this *typically* has the form of
> <1 byte mfr id> <2 byte flash part id>
> 
> I.e.
> @bytes: the bytes returned by the flash when issuing command 9F.
>         Typically, the first byte is the manufacturer ID code (see
>         JEP106) and the next two bytes are a flash part specific ID.
> 

sounds good