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 <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Dan Williams <[email protected]>
---
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);
+
+/**
+ * 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
[ adding Linus for the implications of exporting insert_resource() ]
On Fri, Mar 4, 2016 at 8:14 AM, Toshi Kani <[email protected]> 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 <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Dan Williams <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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