Re: [PATCH 08/15] devlink: add devres managed devlinkm_alloc and devlinkm_free

[Jacob Keller <jacob.e.keller@intel.com>]

On 2/3/2020 3:29 AM, Jiri Pirko wrote:
> Thu, Jan 30, 2020 at 11:59:03PM CET, jacob.e.keller@intel.com wrote:
>> Add devres managed allocation functions for allocating a devlink
>> instance. These can be used by device drivers based on the devres
>> framework which want to allocate a devlink instance.
>>
>> For simplicity and to reduce churn in the devlink core code, the devres
>> management works by creating a node with a double-pointer. The devlink
>> instance is allocated using the normal devlink_alloc and released using
>> the normal devlink_free.
>>
>> An alternative solution where the raw memory for devlink is allocated
>> directly via devres_alloc could be done. Such an implementation would
>> either significantly increase code duplication or code churn in order to
>> refactor the setup from the allocation.
>>
>> The new devres managed allocation function will be used by the ice
>> driver in a following change to implement initial devlink support.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> ---
>> include/net/devlink.h |  4 ++++
>> lib/devres.c          |  1 +
>> net/core/devlink.c    | 54 +++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 59 insertions(+)
>>
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index 63e954241404..1c3540280396 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -858,11 +858,15 @@ struct ib_device;
>> struct net *devlink_net(const struct devlink *devlink);
>> void devlink_net_set(struct devlink *devlink, struct net *net);
>> struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size);
>> +struct devlink *devlinkm_alloc(struct device * dev,
>> +			       const struct devlink_ops *ops,
>> +			       size_t priv_size);
>> int devlink_register(struct devlink *devlink, struct device *dev);
>> void devlink_unregister(struct devlink *devlink);
>> void devlink_reload_enable(struct devlink *devlink);
>> void devlink_reload_disable(struct devlink *devlink);
>> void devlink_free(struct devlink *devlink);
>> +void devlinkm_free(struct device *dev, struct devlink *devlink);
>> int devlink_port_register(struct devlink *devlink,
>> 			  struct devlink_port *devlink_port,
>> 			  unsigned int port_index);
>> diff --git a/lib/devres.c b/lib/devres.c
>> index 6ef51f159c54..239c81d40612 100644
>> --- a/lib/devres.c
>> +++ b/lib/devres.c
>> @@ -5,6 +5,7 @@
>> #include <linux/gfp.h>
>> #include <linux/export.h>
>> #include <linux/of_address.h>
>> +#include <net/devlink.h>
>>
>> enum devm_ioremap_type {
>> 	DEVM_IOREMAP = 0,
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 574008c536fa..b2b855d12a11 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -6531,6 +6531,60 @@ void devlink_free(struct devlink *devlink)
>> }
>> EXPORT_SYMBOL_GPL(devlink_free);
>>
>> +static void devres_devlink_release(struct device *dev, void *res)
>> +{
>> +	devlink_free(*(struct devlink **)res);
>> +}
>> +
>> +static int devres_devlink_match(struct device *dev, void *res, void *data)
>> +{
>> +	return *(struct devlink **)res == data;
>> +}
>> +
>> +/**
>> + * devlinkm_alloc - Allocate devlink instance managed by devres
>> + * @dev: device to allocate devlink for
>> + * @ops: devlink ops structure
>> + * @priv_size: size of private data portion
>> + *
>> + * Allocate a devlink instance and manage its release via devres.
>> + */
>> +struct devlink *devlinkm_alloc(struct device *dev,
> 
> Why "devlinkm"? Looks like the usual prefix for this is "devm_"
> So "devm_devlink_alloc/free"?
> 
> 

pcim_enable_device
pcim_iomap
pcim_iomap_devres
pcim_iomap_regions
pcim_iomap_regions_request_all
pcim_iomap_release
pcim_iomap_table
pcim_iounmap
pcim_iounmap_regions
pcim_pin_device
pcim_release
pcim_set_mwi
pcim_state

There are some devm_pci_* though... Heh.

Regardless, I agree wit Jakub, and am going to remove this, because it
seems less valuable since the driver needs to manage the remaining
devlink state regardless. (Unless we add devm_devlink_* for every thing,
but....)

I think it makes more sense to assume a devres driver would need to
manage its own usage via direct devres calls to setup the teardown
actions manually.

Thanks,
Jake