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

[Toshi Kani <toshi.kani@hpe.com>]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html