Re: [PATCH v2 3/8] mtd: spi-nor: core: Use auto-detection only once

[Michael Walle]

Am 2022-03-21 18:42, schrieb Pratyush Yadav:
> On 21/03/22 12:50PM, Tudor.Ambarus@xxxxxxxxxxxxx wrote:
> > On 3/21/22 14:14, Pratyush Yadav wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > >
> > > On 28/02/22 01:17PM, Tudor Ambarus wrote:
> > >> In case spi_nor_match_name() returned NULL, the auto detection was
> > >> issued twice. There's no reason to try to detect the same chip twice,
> > >> do the auto detection only once.
> > >>
> > >> Signed-off-by: Tudor Ambarus <tudor.ambarus@xxxxxxxxxxxxx>
> > >> ---
> > >>  drivers/mtd/spi-nor/core.c | 10 ++++++----
> > >>  1 file changed, 6 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > >> index f87cb7d3daab..b1d6fa65417d 100644
> > >> --- a/drivers/mtd/spi-nor/core.c
> > >> +++ b/drivers/mtd/spi-nor/core.c
> > >> @@ -2894,13 +2894,15 @@ static const struct flash_info *spi_nor_match_name(struct spi_nor *nor,
> > >>  static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
> > >>                                                      const char *name)
> > >>  {
> > >> -     const struct flash_info *info = NULL;
> > >> +     const struct flash_info *info = NULL, *detected_info = NULL;
> > >>
> > >>       if (name)
> > >>               info = spi_nor_match_name(nor, name);
> > >>       /* Try to auto-detect if chip name wasn't specified or not found */
> > >> -     if (!info)
> > >> -             info = spi_nor_read_id(nor);
> > >> +     if (!info) {
> > >> +             detected_info = spi_nor_read_id(nor);
> > >> +             info = detected_info;
> > >> +     }
> > >>       if (IS_ERR_OR_NULL(info))
> > >>               return ERR_PTR(-ENOENT);
> > >>
> > >> @@ -2908,7 +2910,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 && !detected_info && info->id_len) {
> > >>               const struct flash_info *jinfo;
> > >>
> > >>               jinfo = spi_nor_read_id(nor);
> > >
> > > I think the flow can be a little bit better. How about:
> > >
> > >         if (name)
> > >                 info = spi_nor_match_name();
> > >
> > >         if (!info) {
> > >                 info = spi_nor_read_id();
> > >                 if (IS_ERR_OR_NULL(info))
> > >                         return ERR_PTR(-ENOENT);
> > >
> > >                 return info;
> > >         }

+1 for the flow. But is it correct that we just ignore any former
error and just replace it with ENOENT? Should we return NULL here
and let the caller handle the translation from NULL to ENOENT (and
keeping any other errors)

> > Here we miss the IS_ERR check in case info is retrieved with 
> > spi_nor_match_name().
> > Do you expect spi_nor_match_name() to ever return an error? As it is 
> > now it doesn't.
> > I'm fine either way. In case you want me to follow your suggestion, 
> > give me a sign
> > and I'll make a dedicated patch to move the IS_ERR_OR_NULL check. Will 
> > add your
> > Suggested-by tag.
>
> I think it should be safe to assume it won't ever return an error since
> all it does is iterate over an array that is always present. I don't 
> see
> that changing in the foreseeable future either. So I think not having
> the IS_ERR check is fine.

But what does it cost to just add the error check now so it won't
be forgotten in the future?

         if (name) {
                 info = spi_nor_match_name();
                 if (IS_ERR(info))
                         return info;
         }
         if (!info)
                  return spi_nor_read_id();

         <flash model check code follows here>

And then let the caller handle NULL and translate it to ENOENT.

-michael