Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752262AbdCCONj (ORCPT ); Fri, 3 Mar 2017 09:13:39 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:36780 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751160AbdCCONh (ORCPT ); Fri, 3 Mar 2017 09:13:37 -0500 Date: Fri, 3 Mar 2017 15:03:31 +0100 From: Boris Brezillon To: Alban Cc: Srinivas Kandagatla , 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 , Moritz Fischer Subject: Re: [PATCH 2/3] mtd: Add support for reading MTD devices via the nvmem API Message-ID: <20170303150331.702e0e41@bbrezillon> In-Reply-To: <20170303143021.46381429@tock> References: <1488484223-844-1-git-send-email-albeu@free.fr> <1488484223-844-3-git-send-email-albeu@free.fr> <20170303133419.2383dbce@bbrezillon> <20170303143021.46381429@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 Content-Length: 3030 Lines: 79 On Fri, 3 Mar 2017 14:30:21 +0100 Alban wrote: > On Fri, 3 Mar 2017 13:34:19 +0100 > Boris Brezillon wrote: > > > On Fri, 3 Mar 2017 11:23:16 +0000 > > Srinivas Kandagatla wrote: > > > > > > > > > > > + 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"); > > > > > > I will be nice to feedback error to top layer, as it does not make sense > > > to remove providers if there are active consumers using it. > > > > > > del_mtd_device(), unregister_mtd_user() have return values, I see no > > > reason why notifiers should not return errors. > > > May be if we should fix the remove() call backs to handle and return errors. > > > > It's more complicated than that. What should you do if one of the > > ->remove() notifier in the middle of the list is returning an error? > > Some of them have already taken the remove notification into account. > > Should we call ->add() back on those notifiers? Also, I'm not sure they > > are all safe against double ->remove() calls, so if we might be in > > trouble when the removal is retried. > > Re-adding make no sense as that could also fails. I agree. > Keep it simple, > remove the notifier from the list when remove() succeed, abort when one > fails. In such a scenario that mean there is a dependency, the sys > admin should then solve this dependency and re-trigger the MTD removal. Except notifiers are by definition not attached to a specific MTD device. I get your point, but I think we should clarify the different concepts. An mtd_notifier (which seems to also be called a user in a few places) is something that should be called each time you have an MTD creation/removal event (or when you add a notifier to the list). You could have notifiers that don't do anything special with the MTD device, hence they don't require private data. I think we should add the mtd_user concept, which would be a specific user of an MTD device that can contain private data and is likely to be attached to the MTD device after the notifier's ->add() method is called. struct mtd_user_ops { int (*remove)(struct mtd_user *); }; struct mtd_user { struct list_node node; const struct mtd_user_ops *ops; } int mtd_attach_user(struct mtd_info *mtd, struct mtd_user *user); int mtd_detach_user(struct mtd_info *mtd, struct mtd_user *user); and then inside the del_mtd_device() function, before you iterate over all notifiers, you could iterate over all attached users and call their ->remove() method. If one fails, then you stop the removal procedure.