Re: [PATCH v2-UPDATE 3/4] resource: Add device-managed insert/remove_resource()

[Toshi Kani]

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