4:14 p.m.
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(a)hpe.com>
Cc: Ingo Molnar <mingo(a)kernel.org>
Cc: Borislav Petkov <bp(a)suse.de>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: Dan Williams <dan.j.williams(a)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);
+
+/**
+ * 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
6:14 p.m.
[ adding Linus for the implications of exporting insert_resource() ]
On Fri, Mar 4, 2016 at 8:14 AM, Toshi Kani <toshi.kani(a)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(a)hpe.com>
Cc: Ingo Molnar <mingo(a)kernel.org>
Cc: Borislav Petkov <bp(a)suse.de>
Cc: Andrew Morton <akpm(a)linux-foundation.org>
Cc: Dan Williams <dan.j.williams(a)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.
11:03 p.m.
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
2:21 a.m.
On Mon, Mar 7, 2016 at 3:03 PM, Toshi Kani <toshi.kani(a)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?
12:02 p.m.
* Toshi Kani <toshi.kani(a)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
5:41 p.m.
On Tue, 2016-03-08 at 13:02 +0100, Ingo Molnar wrote:
* Toshi Kani <toshi.kani(a)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
2352
days inactive
2356
days old
5 comments
3 participants
participants (3)
-
Dan Williams -
Ingo Molnar -
Toshi Kani