Re: [PATCH 2/2] mtd: spi-nor: add initial sysfs support

[Michael Walle]

Hi Alex,

Am 2021-04-29 17:37, schrieb Alexander Williams:
> Hi Michael,
>
> > On Thu, Mar 18, 2021 at 10:24 AM Michael Walle <michael@xxxxxxxx>
> > wrote:
> > Add support to show the name and JEDEC identifier as well as to dump 
> > the
> > SFDP table. Not all flashes list their SFDP table contents in their
> > datasheet. So having that is useful. It might also be helpful in bug
> > reports from users.
> >
> > The idea behind the sysfs module is also to have raw access to the SPI
> > NOR flash device registers, which can also be useful for debugging.
> >
> > Signed-off-by: Michael Walle <michael@xxxxxxxx>
> > ---
> >  drivers/mtd/spi-nor/Makefile |  2 +-
> >  drivers/mtd/spi-nor/core.c   |  5 +++
> >  drivers/mtd/spi-nor/core.h   |  3 ++
> >  drivers/mtd/spi-nor/sysfs.c  | 86 
> > ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 95 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mtd/spi-nor/sysfs.c
> >
> > diff --git a/drivers/mtd/spi-nor/Makefile 
> > b/drivers/mtd/spi-nor/Makefile
> > index 653923896205..aff308f75987 100644
> > --- a/drivers/mtd/spi-nor/Makefile
> > +++ b/drivers/mtd/spi-nor/Makefile
> > @@ -1,6 +1,6 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >
> > -spi-nor-objs			:= core.o sfdp.o
> > +spi-nor-objs			:= core.o sfdp.o sysfs.o
> >  spi-nor-objs			+= atmel.o
> >  spi-nor-objs			+= catalyst.o
> >  spi-nor-objs			+= eon.o
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 4a315cb1c4db..2eaf4ba8c0f3 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -3707,6 +3707,10 @@ static int spi_nor_probe(struct spi_mem 
> > *spimem)
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = spi_nor_sysfs_create(nor);
>
> This appears to be racing with user space. By the time we reach 
> probe(), the
> device embedded in the spi_device has already been registered, with the 
> uevent
> sent out, right? udev may not see the new attributes created here.
>
> Since we reuse a preexisting device throughout spi-nor, it seems 
> awfully
> challenging to be able to safely add sysfs attributes. Would it make 
> sense to
> create a spi-nor-specific device/class? Or perhaps attach these 
> attributes to
> the device in mtd_info like I've done in
> https://lore.kernel.org/linux-mtd/20210428052725.530939-1-awill@xxxxxxxxxx/ 
> ?

Do you encounter this problem? I'm currently working on this and dropped
the sysfs_create() and use dev_groups of the driver spi nor driver.

But I'm not sure how it will be handled anyway. Because we know the
content/size SFDP only after the probe and in any case the probe could
also fail. So I don't really understand how that is handled.

I've looked at your patch and it seems that the surpress_uevent() is
rarely used in the kernel.

I don't want to attach it to an MTD device because you might have
multiple ones which has the same SPI flash device as parent. The
SFDP is really a property of the flash device and not one of the
MTD partition.

-michael