From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751902AbdCCNjp (ORCPT ); Fri, 3 Mar 2017 08:39:45 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:35582 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751050AbdCCNjY (ORCPT ); Fri, 3 Mar 2017 08:39:24 -0500 Date: Fri, 3 Mar 2017 14:36:58 +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: <20170303143658.5d89a329@bbrezillon> In-Reply-To: <20170303133629.3aac2945@tock> References: <1488484223-844-1-git-send-email-albeu@free.fr> <1488484223-844-3-git-send-email-albeu@free.fr> <20170302221803.223ff23b@bbrezillon> <20170303133629.3aac2945@tock> 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 On Fri, 3 Mar 2017 13:36:29 +0100 Alban wrote: > On Thu, 2 Mar 2017 22:18:03 +0100 > Boris Brezillon wrote: > > > On Thu, 2 Mar 2017 20:50:22 +0100 > > Alban wrote: > > > > [snip] > > > > > +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. > > Yes to support such a binding we would have to fix of_nvmem_cell_get(), > but that should be quiet simple to have it support both the new and old > binding. > > > > > [snip] > > > > > +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. > > Yes, I'm aware of this problem. Sorry, I forgot to mention this in the > cover letter. No problem. > > > 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. > > IMHO the MTD users framework has to be re-worked to be useful. First > both the add and remove callbacks should have return values. Users where > the add failed shouldn't be removed later and users where the remove > fails should block the removal of the MTD. As said in my previous reply, it's not just about returning an error. I had a closer look at the code, and it seems that using __get_mtd_device() properly should prevent the problem we are talking about (call __get_mtd_device() after your nvmem_register() and call __put_mtd_device() only if nvmem_unregister() succeed). > > Furthermore only passing the MTD device to the add/remove callback > force the users to keep their own list, which is annoying to say the > least. A simple fix would be to have the add callback return a pointer > that would be passed back to the remove callback. Trivial to implement > and the MTD user wouldn't have to keep any list. I will look into this > in the next days. That's a different problem, and I'm not sure I like the idea of changing the ->add() prototype into void *(*add)(struct mtd_info *); If we want to do that, I'd rather see an API extension allowing one to attach/detach/query/update user data to an MTD device.