Message-ID: <20170302221803.223ff23b@bbrezillon>
Date:   Thu, 2 Mar 2017 22:18:03 +0100
From:   Boris Brezillon <boris.brezillon@...e-electrons.com>
To:     Alban <albeu@...e.fr>
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-mtd@...ts.infradead.org,
        Cyrille Pitchen <cyrille.pitchen@...el.com>,
        Richard Weinberger <richard@....at>,
        Marek Vasut <marek.vasut@...il.com>,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Mark Rutland <mark.rutland@....com>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Moritz Fischer <moritz.fischer@...us.com>
Subject: Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the
 nvmem API

On Thu,  2 Mar 2017 20:50:22 +0100
Alban <albeu@...e.fr> wrote:

> Allow drivers that use the nvmem API to read data stored on MTD devices.
> This add a simple mtd user that register itself as a read-only nvmem
> device.
> 
> Signed-off-by: Alban <albeu@...e.fr>

Just a few comments, but it looks pretty good already.

> ---
>  drivers/mtd/Kconfig    |   9 ++++
>  drivers/mtd/Makefile   |   1 +
>  drivers/mtd/mtdnvmem.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 131 insertions(+)
>  create mode 100644 drivers/mtd/mtdnvmem.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..9cad86c 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -322,6 +322,15 @@ config MTD_PARTITIONED_MASTER
>  	  the parent of the partition device be the master device, rather than
>  	  what lies behind the master.
>  
> +config MTD_NVMEM
> +	tristate "Read config data from MTD devices"
> +	default y
> +	depends on NVMEM
> +	help
> +	  Provides support for reading config data from MTD devices. This can
> +	  be used by drivers to read device specific data such as MAC addresses
> +	  or calibration results.
> +
>  source "drivers/mtd/chips/Kconfig"
>  
>  source "drivers/mtd/maps/Kconfig"
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..f62f50b 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_SSFDC)		+= ssfdc.o
>  obj-$(CONFIG_SM_FTL)		+= sm_ftl.o
>  obj-$(CONFIG_MTD_OOPS)		+= mtdoops.o
>  obj-$(CONFIG_MTD_SWAP)		+= mtdswap.o
> +obj-$(CONFIG_MTD_NVMEM)		+= mtdnvmem.o
>  
>  nftl-objs		:= nftlcore.o nftlmount.o
>  inftl-objs		:= inftlcore.o inftlmount.o
> diff --git a/drivers/mtd/mtdnvmem.c b/drivers/mtd/mtdnvmem.c
> new file mode 100644
> index 0000000..6eb4216
> --- /dev/null
> +++ b/drivers/mtd/mtdnvmem.c
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (C) 2017 Alban Bedel <albeu@...e.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +
> +struct mtd_nvmem {
> +	struct list_head list;
> +	struct mtd_info *mtd;
> +	struct nvmem_device *nvmem;
> +};
> +
> +static DEFINE_MUTEX(mtd_nvmem_list_lock);
> +static LIST_HEAD(mtd_nvmem_list);

I was wondering if we should have the nvmem pointer directly embedded
in the mtd_info struct. Your approach has the benefit of keeping then
nvmem wrapper completely independent, which is a good thing, but you'll
see below that there's a problem with the MTD notifier approach.

> +
> +static int mtd_nvmem_reg_read(void *priv, unsigned int offset,
> +			      void *val, size_t bytes)
> +{
> +	struct mtd_info *mtd = priv;
> +	size_t retlen;
> +	int err;
> +
> +	err = mtd_read(mtd, offset, bytes, &retlen, val);
> +	if (err && err != -EUCLEAN)
> +		return err;
> +
> +	return retlen == bytes ? 0 : -EIO;
> +}
> +
> +static void mtd_nvmem_add(struct mtd_info *mtd)
> +{
> +	struct device *dev = &mtd->dev;
> +	struct device_node *np = dev_of_node(dev);
> +	struct nvmem_config config = {};
> +	struct mtd_nvmem *mtd_nvmem;
> +
> +	/* OF devices have to provide the nvmem node */
> +	if (np && !of_property_read_bool(np, "nvmem-provider"))
> +		return;

Might have to be adapted according to the DT binding if we decide to
add an extra subnode, but then, I'm not sure the nvmem cells creation
will work correctly, because the framework expect nvmem cells to be
direct children of the nvmem device, which will no longer be the case
if you add an intermediate node between the MTD device node and the
nvmem cell nodes.

> +
> +	config.dev = dev;
> +	config.owner = THIS_MODULE;
> +	config.reg_read = mtd_nvmem_reg_read;
> +	config.size = mtd->size;
> +	config.word_size = 1;
> +	config.stride = 1;
> +	config.read_only = true;
> +	config.priv = mtd;
> +
> +	/* Alloc our struct to keep track of the MTD NVMEM devices */
> +	mtd_nvmem = kzalloc(sizeof(*mtd_nvmem), GFP_KERNEL);
> +	if (!mtd_nvmem)
> +		return;
> +
> +	mtd_nvmem->mtd = mtd;
> +	mtd_nvmem->nvmem = nvmem_register(&config);
> +	if (IS_ERR(mtd_nvmem->nvmem)) {
> +		dev_err(dev, "Failed to register NVMEM device\n");
> +		kfree(mtd_nvmem);
> +		return;
> +	}
> +
> +	mutex_lock(&mtd_nvmem_list_lock);
> +	list_add_tail(&mtd_nvmem->list, &mtd_nvmem_list);
> +	mutex_unlock(&mtd_nvmem_list_lock);
> +}
> +
> +static void mtd_nvmem_remove(struct mtd_info *mtd)
> +{
> +	struct mtd_nvmem *mtd_nvmem;
> +	bool found = false;
> +
> +	mutex_lock(&mtd_nvmem_list_lock);
> +	list_for_each_entry(mtd_nvmem, &mtd_nvmem_list, list) {
> +		if (mtd_nvmem->mtd == mtd) {
> +			list_del(&mtd_nvmem->list);
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&mtd_nvmem_list_lock);
> +
> +	if (found) {
> +		if (nvmem_unregister(mtd_nvmem->nvmem))
> +			dev_err(&mtd->dev,
> +				"Failed to unregister NVMEM device\n");

Ouch! You failed to unregister the NVMEM device but you have no way to
stop MTD dev removal, which means you have a potential use-after-free
bug. Not sure this can happen in real life, but I don't like that.

Maybe we should let notifiers return an error if they want to cancel
the removal, or maybe this is a good reason to put the nvmem pointer
directly in mtd_info and call mtd_nvmem_add/remove() directly from
add/del_mtd_device() and allow them to return an error.

Not that, if you go for this solution, you'll also get rid of the
global mtd_nvmem_list list and the associated lock.

> +		kfree(mtd_nvmem);
> +	}
> +}
> +
> +static struct mtd_notifier mtd_nvmem_notifier = {
> +	.add = mtd_nvmem_add,
> +	.remove = mtd_nvmem_remove,
> +};
> +
> +static int __init mtd_nvmem_init(void)
> +{
> +	register_mtd_user(&mtd_nvmem_notifier);
> +	return 0;
> +}
> +module_init(mtd_nvmem_init);
> +
> +static void __exit mtd_nvmem_exit(void)
> +{
> +	unregister_mtd_user(&mtd_nvmem_notifier);
> +}
> +module_exit(mtd_nvmem_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Alban Bedel <albeu@...e.fr>");
> +MODULE_DESCRIPTION("Driver to read config data from MTD devices");

Powered by blists - more mailing lists