Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751492AbdCBVTI (ORCPT ); Thu, 2 Mar 2017 16:19:08 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:52348 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751009AbdCBVSz (ORCPT ); Thu, 2 Mar 2017 16:18:55 -0500 Date: Thu, 2 Mar 2017 22:18:03 +0100 From: Boris Brezillon To: Alban Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-mtd@lists.infradead.org, Cyrille Pitchen , Richard Weinberger , Marek Vasut , Brian Norris , David Woodhouse , Mark Rutland , Rob Herring , Maxime Ripard , Srinivas Kandagatla , Moritz Fischer Subject: Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API Message-ID: <20170302221803.223ff23b@bbrezillon> In-Reply-To: <1488484223-844-3-git-send-email-albeu@free.fr> References: <1488484223-844-1-git-send-email-albeu@free.fr> <1488484223-844-3-git-send-email-albeu@free.fr> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6357 Lines: 204 On Thu, 2 Mar 2017 20:50:22 +0100 Alban 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 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > + > +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 "); > +MODULE_DESCRIPTION("Driver to read config data from MTD devices");