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

From: Michael Walle
Date: Thu Apr 29 2021 - 11:46:43 EST

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

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.