by Alexander Williams

[permalink] [raw]

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

Hi Michael,

> On Thu, Mar 18, 2021 at 10:24 AM Michael Walle <[email protected]>
> 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 <[email protected]>
> ---
> 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/[email protected]/ ?

- Alex

> + if (ret)
> + return ret;
> +
> return mtd_device_register(&nor->mtd, data ? data->parts : NULL,
> data ? data->nr_parts : 0);
> }
> @@ -3716,6 +3720,7 @@ static int spi_nor_remove(struct spi_mem *spimem)
> struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>
> spi_nor_restore(nor);
> + spi_nor_sysfs_remove(nor);
>
> /* Clean up MTD stuff. */
> return mtd_device_unregister(&nor->mtd);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 668f22011b1d..dd592f7b62d1 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -488,4 +488,7 @@ static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
> return mtd->priv;
> }
>
> +int spi_nor_sysfs_create(struct spi_nor *nor);
> +void spi_nor_sysfs_remove(struct spi_nor *nor);
> +
> #endif /* __LINUX_MTD_SPI_NOR_INTERNAL_H */
> diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
> new file mode 100644
> index 000000000000..0de031e246c5
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -0,0 +1,86 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/mtd/spi-nor.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/spi-mem.h>
> +#include <linux/sysfs.h>
> +
> +#include "core.h"
> +
> +static ssize_t name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + return sprintf(buf, "%s\n", nor->info->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static ssize_t jedec_id_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + return sprintf(buf, "%*phN\n", nor->info->id_len, nor->info->id);
> +}
> +static DEVICE_ATTR_RO(jedec_id);
> +
> +static struct attribute *spi_nor_sysfs_entries[] = {
> + &dev_attr_name.attr,
> + &dev_attr_jedec_id.attr,
> + NULL
> +};
> +
> +static ssize_t sfdp_read(struct file *filp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf,
> + loff_t off, size_t count)
> +{
> + struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> + struct sfdp *sfdp = nor->sfdp;
> + size_t sfdp_size = sfdp->num_dwords * sizeof(*sfdp->dwords);
> +
> + return memory_read_from_buffer(buf, count, &off, nor->sfdp->dwords,
> + sfdp_size);
> +}
> +static BIN_ATTR_RO(sfdp, PAGE_SIZE);
> +
> +static struct bin_attribute *spi_nor_sysfs_bin_entries[] = {
> + &bin_attr_sfdp,
> + NULL
> +};
> +
> +static umode_t spi_nor_sysfs_is_bin_visible(struct kobject *kobj,
> + struct bin_attribute *attr, int n)
> +{
> + struct spi_device *spi = to_spi_device(kobj_to_dev(kobj));
> + struct spi_mem *spimem = spi_get_drvdata(spi);
> + struct spi_nor *nor = spi_mem_get_drvdata(spimem);
> +
> + if (attr == &bin_attr_sfdp && nor->sfdp)
> + return 0444;
> +
> + return 0;
> +}
> +
> +static struct attribute_group spi_nor_sysfs_attr_group = {
> + .name = NULL,
> + .is_bin_visible = spi_nor_sysfs_is_bin_visible,
> + .attrs = spi_nor_sysfs_entries,
> + .bin_attrs = spi_nor_sysfs_bin_entries,
> +};
> +
> +int spi_nor_sysfs_create(struct spi_nor *nor)
> +{
> + return sysfs_create_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> +
> +void spi_nor_sysfs_remove(struct spi_nor *nor)
> +{
> + sysfs_remove_group(&nor->dev->kobj, &spi_nor_sysfs_attr_group);
> +}
> --
> 2.20.1

2021-04-29 15:47:29

by Michael Walle

[permalink] [raw]

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

Hi Alex,

Am 2021-04-29 17:37, schrieb Alexander Williams:
> Hi Michael,
>
>> On Thu, Mar 18, 2021 at 10:24 AM Michael Walle <[email protected]>
>> 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 <[email protected]>
>> ---
>> 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/[email protected]/
> ?

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