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

[Dan Williams <dan.j.williams@intel.com>]

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?