Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932250AbcCHCVk (ORCPT ); Mon, 7 Mar 2016 21:21:40 -0500 Received: from mail-ob0-f179.google.com ([209.85.214.179]:34892 "EHLO mail-ob0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932155AbcCHCVb (ORCPT ); Mon, 7 Mar 2016 21:21:31 -0500 MIME-Version: 1.0 In-Reply-To: <1457391787.15454.399.camel@hpe.com> References: <1457108082-4610-1-git-send-email-toshi.kani@hpe.com> <1457391787.15454.399.camel@hpe.com> Date: Mon, 7 Mar 2016 18:21:28 -0800 Message-ID: Subject: Re: [PATCH v2-UPDATE 3/4] resource: Add device-managed insert/remove_resource() From: Dan Williams To: Toshi Kani 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" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3813 Lines: 97 On Mon, Mar 7, 2016 at 3:03 PM, Toshi Kani wrote: > 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, this satisfies my concerns. Ingo, any concern with me taking this series through the nvdimm tree since it touches the nfit driver?