| Message ID | 1457108082-4610-1-git-send-email-toshi.kani@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[ adding Linus for the implications of exporting insert_resource() ] On Fri, Mar 4, 2016 at 8:14 AM, Toshi Kani <toshi.kani@hpe.com> wrote: > insert_resource() and remove_resouce() are called by producers > of resources, such as FW modules and bus drivers. These modules > may be implemented as loadable modules. > > Add device-managed implementaions of insert_resource() and > remove_resouce() functions. > > Signed-off-by: Toshi Kani <toshi.kani@hpe.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Borislav Petkov <bp@suse.de> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Dan Williams <dan.j.williams@intel.com> > --- > v2-UPDATE: > - Rename a helper remove func to __devm_remove_resource(). (Dan Williams) > --- > include/linux/ioport.h | 5 +++ > kernel/resource.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+) > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index 8017b8b..3580038 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -259,6 +259,11 @@ extern struct resource * __devm_request_region(struct device *dev, > > extern void __devm_release_region(struct device *dev, struct resource *parent, > resource_size_t start, resource_size_t n); > + > +extern int devm_insert_resource(struct device *dev, struct resource *root, > + struct resource *new); > +extern void devm_remove_resource(struct device *dev, struct resource *old); > + > extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size); > extern int iomem_is_exclusive(u64 addr); > > diff --git a/kernel/resource.c b/kernel/resource.c > index effb6ee..12a9d57 100644 > --- a/kernel/resource.c > +++ b/kernel/resource.c > @@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev, struct resource *parent, > EXPORT_SYMBOL(__devm_release_region); > > /* > + * Helper remove function for devm_insert_resource() and devm_remove_resource() > + */ > +static void __devm_remove_resource(struct device *dev, void *ptr) > +{ > + struct resource **r = ptr; > + > + remove_resource(*r); > +} > + > +/** > + * 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.
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
On Mon, Mar 7, 2016 at 3:03 PM, Toshi Kani <toshi.kani@hpe.com> 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?
* Toshi Kani <toshi.kani@hpe.com> 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 s/explicitly since /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; Why not return 'ret' here, instead of -EBUSY? > + } > + > + devres_add(dev, ptr); > + return 0; > +} > +EXPORT_SYMBOL_GPL(devm_insert_resource); > + > +/** > + * devm_remove_resource() - remove a previously inserted resource > + * @dev: device for which to remove the resource > + * @old: descriptor of the resource > + * > + * Remove a resource previously inserted using devm_insert_resource(). > + * > + * devm_remove_resource() is intended for producers of resources, such as > + * FW modules and bus drivers. > + */ > +void devm_remove_resource(struct device *dev, struct resource *old) > +{ > + WARN_ON(devres_release(dev, __devm_remove_resource, devm_resource_match, > + old)); So generally we don't put functions with side effects into WARN_ON()s. Just like BUG_ON(), in the future it might be disabled on certain Kconfigs, etc. - and it's also bad for readability. Also, please use WARN_ON_ONCE(). > +} > +EXPORT_SYMBOL_GPL(devm_remove_resource); > + > +/* > * Called from init/main.c to reserve IO ports. > */ > #define MAXRESERVE 4 Looks good to me otherwise. Thanks, Ingo
On Tue, 2016-03-08 at 13:02 +0100, Ingo Molnar wrote: > * Toshi Kani <toshi.kani@hpe.com> 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 > > s/explicitly since > /explicitly, since Will do. > > + * 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; > > Why not return 'ret' here, instead of -EBUSY? Right, I will change it to 'return ret'. > > + } > > + > > + devres_add(dev, ptr); > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(devm_insert_resource); > > + > > +/** > > + * devm_remove_resource() - remove a previously inserted resource > > + * @dev: device for which to remove the resource > > + * @old: descriptor of the resource > > + * > > + * Remove a resource previously inserted using devm_insert_resource(). > > + * > > + * devm_remove_resource() is intended for producers of resources, such > > as > > + * FW modules and bus drivers. > > + */ > > +void devm_remove_resource(struct device *dev, struct resource *old) > > +{ > > + WARN_ON(devres_release(dev, __devm_remove_resource, > > devm_resource_match, > > + old)); > > So generally we don't put functions with side effects into WARN_ON()s. > Just like BUG_ON(), in the future it might be disabled on certain > Kconfigs, etc. - and it's also bad for readability. > > Also, please use WARN_ON_ONCE(). I see. Will change to test with WARN_ON_ONCE(ret). > > +} > > +EXPORT_SYMBOL_GPL(devm_remove_resource); > > + > > +/* > > * Called from init/main.c to reserve IO ports. > > */ > > #define MAXRESERVE 4 > > Looks good to me otherwise. Great! I will send an updated patch as "[PATCH v2-UPDATE2 3/4]". Thanks, -Toshi
diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 8017b8b..3580038 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -259,6 +259,11 @@ extern struct resource * __devm_request_region(struct device *dev, extern void __devm_release_region(struct device *dev, struct resource *parent, resource_size_t start, resource_size_t n); + +extern int devm_insert_resource(struct device *dev, struct resource *root, + struct resource *new); +extern void devm_remove_resource(struct device *dev, struct resource *old); + extern int iomem_map_sanity_check(resource_size_t addr, unsigned long size); extern int iomem_is_exclusive(u64 addr); diff --git a/kernel/resource.c b/kernel/resource.c index effb6ee..12a9d57 100644 --- a/kernel/resource.c +++ b/kernel/resource.c @@ -1449,6 +1449,75 @@ void __devm_release_region(struct device *dev, struct resource *parent, EXPORT_SYMBOL(__devm_release_region); /* + * Helper remove function for devm_insert_resource() and devm_remove_resource() + */ +static void __devm_remove_resource(struct device *dev, void *ptr) +{ + struct resource **r = ptr; + + remove_resource(*r); +} + +/** + * 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); + +/** + * devm_remove_resource() - remove a previously inserted resource + * @dev: device for which to remove the resource + * @old: descriptor of the resource + * + * Remove a resource previously inserted using devm_insert_resource(). + * + * devm_remove_resource() is intended for producers of resources, such as + * FW modules and bus drivers. + */ +void devm_remove_resource(struct device *dev, struct resource *old) +{ + WARN_ON(devres_release(dev, __devm_remove_resource, devm_resource_match, + old)); +} +EXPORT_SYMBOL_GPL(devm_remove_resource); + +/* * Called from init/main.c to reserve IO ports. */ #define MAXRESERVE 4
insert_resource() and remove_resouce() are called by producers of resources, such as FW modules and bus drivers. These modules may be implemented as loadable modules. Add device-managed implementaions of insert_resource() and remove_resouce() functions. Signed-off-by: Toshi Kani <toshi.kani@hpe.com> Cc: Ingo Molnar <mingo@kernel.org> Cc: Borislav Petkov <bp@suse.de> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Dan Williams <dan.j.williams@intel.com> --- v2-UPDATE: - Rename a helper remove func to __devm_remove_resource(). (Dan Williams) --- include/linux/ioport.h | 5 +++ kernel/resource.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+)