Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753501AbcCGWKi (ORCPT ); Mon, 7 Mar 2016 17:10:38 -0500 Received: from g9t5009.houston.hp.com ([15.240.92.67]:33249 "EHLO g9t5009.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753099AbcCGWKa (ORCPT ); Mon, 7 Mar 2016 17:10:30 -0500 Message-ID: <1457391787.15454.399.camel@hpe.com> Subject: Re: [PATCH v2-UPDATE 3/4] resource: Add device-managed insert/remove_resource() From: Toshi Kani To: Dan Williams Cc: Ingo Molnar , Borislav Petkov , "Rafael J. Wysocki" , Andrew Morton , "linux-nvdimm@lists.01.org" , Linux ACPI , "linux-kernel@vger.kernel.org" , "torvalds@linux-foundation.org" Date: Mon, 07 Mar 2016 16:03:07 -0700 In-Reply-To: References: <1457108082-4610-1-git-send-email-toshi.kani@hpe.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.4 (3.18.4-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3666 Lines: 94 On Mon, 2016-03-07 at 10:14 -0800, Dan Williams wrote:  : > > +/** > > + * devm_insert_resource() - insert an I/O or memory resource > > + * @dev: device for which to produce the resource > > + * @root: root of the resource tree > > + * @new: descriptor of the new resource > > + * > > + * This is a device-managed version of insert_resource(). There is > > usually > > + * no need to release resources requested by this function explicitly > > since > > + * that will be taken care of when the device is unbound from its bus > > driver. > > + * If for some reason the resource needs to be released explicitly, > > because > > + * of ordering issues for example, bus drivers must call > > devm_remove_resource() > > + * rather than the regular remove_resource(). > > + * > > + * devm_insert_resource() is intended for producers of resources, such > > as > > + * FW modules and bus drivers. > > + * > > + * Returns 0 on success or a negative error code on failure. > > + */ > > +int devm_insert_resource(struct device *dev, struct resource *root, > > +                         struct resource *new) > > +{ > > +       struct resource **ptr; > > +       int ret; > > + > > +       ptr = devres_alloc(__devm_remove_resource, sizeof(*ptr), > > GFP_KERNEL); > > +       if (!ptr) > > +               return -ENOMEM; > > + > > +       *ptr = new; > > + > > +       ret = insert_resource(root, new); > > +       if (ret) { > > +               dev_err(dev, "unable to insert resource: %pR (%d)\n", > > new, ret); > > +               devres_free(ptr); > > +               return -EBUSY; > > +       } > > + > > +       devres_add(dev, ptr); > > +       return 0; > > +} > > +EXPORT_SYMBOL_GPL(devm_insert_resource); > > The only hesitation I have with this is that the kernel has gotten by > a long time without allowing external modules to insert resources.  If > keeping it private all this time was purposeful then maybe we should > make this new NVDIMM-need to call insert_resource() private to the > nfit driver and built-in-only. Here is some background info on this: - External modules can already insert resources via platform_device_add(), which is used for inserting resources that may not be enumerated by standard FW interfaces.  There are over 200 callers already. - PCI mmcfg driver and ACPI HPET driver find their resources from ACPI, and call insert_resource() to insert them, which is similar to what this patchset tries to do with ACPI NFIT.  Both PCI and HPET drivers do not support unloading, however.   # cat /proc/iomem    :   80000000-8fffffff : PCI MMCONFIG 0000 [bus 00-ff]     80000000-8fffffff : reserved    :   fed00000-fed003ff : HPET 0     fed00000-fed003ff : PNP0103:00 - Existing FW modules and bus drivers using insert_resource() are built- into the kernel, and insert_resource() is not exported.  I think this is because these modules did not have to (or may not) support unloading. - Both the NFIT driver and NVDIMM bus drivers are new and support unloading.  This calls for an exported interface for insert_resource(). - devm impl of insert/remove_resource() is added to assure that resources are properly released on unloading.  Exporting devm interfaces makes sense (to me). - However, devm_insert/remove_resource() should not be confused with devm_request/release_resource().  Hence, this patchset adds comments to clarify that they are used for producers of resources.  The same comments are added to insert/remove_resource() as well. Thanks, -Toshi