Re: [PATCH] nvmem: core: add managed version of nvmem_register

[Heiner Kallweit]

Am 08.06.2017 um 08:26 schrieb Srinivas Kandagatla:
> 
> 
> On 07/06/17 22:55, Heiner Kallweit wrote:
>> Am 07.06.2017 um 18:19 schrieb Srinivas Kandagatla:
>>>
>>> On 04/06/17 12:06, Heiner Kallweit wrote:
>>>> Add a device-managed version of nvmem_register.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>> ---
>>>>   Documentation/nvmem/nvmem.txt  |  1 +
>>>>   drivers/nvmem/core.c           | 35 +++++++++++++++++++++++++++++++++++
>>>>   include/linux/nvmem-provider.h |  7 +++++++
>>>>   3 files changed, 43 insertions(+)
>>>>
>>>
>>> Thanks for the patch, one comments..
>>>> diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt
>>>> index dbd40d87..b4ff7862 100644
>>>> --- a/Documentation/nvmem/nvmem.txt
>>>> +++ b/Documentation/nvmem/nvmem.txt
>>>> @@ -37,6 +37,7 @@ and write the non-volatile memory.
>>>>   A NVMEM provider can register with NVMEM core by supplying relevant
>>>>   nvmem configuration to nvmem_register(), on success core would return a valid
>>>>   nvmem_device pointer.
>>>> +devm_nvmem_register() is a device-managed version of nvmem_register.
>>>>
>>>>   nvmem_unregister(nvmem) is used to unregister a previously registered provider.
>>>>
>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>> index 783eb431..55db219f 100644
>>>> --- a/drivers/nvmem/core.c
>>>> +++ b/drivers/nvmem/core.c
>>>> @@ -531,6 +531,41 @@ int nvmem_unregister(struct nvmem_device *nvmem)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(nvmem_unregister);
>>>>
>>>> +static void devm_nvmem_release(struct device *dev, void *res)
>>>> +{
>>>> +    nvmem_unregister(*(struct nvmem_device **)res);
>>>
>>> nvmem_unregister() can fail, how are you going to deal with this error cases?
>>>
>> As stated in my answer to your other review comment:
>> Currently no caller of nvmem_unregister checks the return code.
> Currently all nvmem provider drivers check return code of unregister in remove path.

Sorry, I was referring to at24/at25 etc. only. You're right.

When using the poposed devm_nvmem_register the refcount check should never fail.
devm_ functions in general should be called from the probe function of a driver only.
So the related devm release function is called only after the remove callback.
Every nvmem consumer increases the module refcount (provided that owner is properly
set to THIS_MODULE), so the devm release function is called only when there's no
nvmem consumer any longer.

In case this explanation should be fine with you, I'd resubmit considering your other
comment to explicitely make struct device * the first argument in devm_nvmem_register.

>> Checking the refcount I see more as a debug feature and I think making
> 
> No, I don't think its a debug feature!! without this check we would end up dereferencing a freed pointer.

However triggering this check would clearly indicate a driver bug, either in the nvmem provider
or consumer, don't you think so?
If we leave the refcount check in I'd propose to add such a WARN_ON because the trace should facilitate
bug analysis.

>> nvmem_unregister return void plus a WARN() if refcount != 0 would be better.
> 
> 
> WARN() would not be enough to stop the system to crash, in case the provider just unregisters with active users.
>>
>> Rgds, Heiner
>>
>>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * devm_nvmem_register() - managed version of nvmem_register
>>>> + *
>>>> + * @config: nvmem device configuration with which nvmem device is created.
>>>> + *
>>>> + * Return: Will be an ERR_PTR() on error or a valid pointer to nvmem_device
>>>> + * on success.
>>>> + */
>>>> +
>>>> +struct nvmem_device *devm_nvmem_register(const struct nvmem_config *config)
>>>
>>> For consistency reasons, devm versions of apis should always have dev at as first argument.
>>>
>>>> +{
>>>> +    struct nvmem_device *nv, **dr;
>>>> +
>>>> +    dr = devres_alloc(devm_nvmem_release, sizeof(*dr), GFP_KERNEL);
>>>> +    if (!dr)
>>>> +        return ERR_PTR(-ENOMEM);
>>>> +
>>>> +    nv = nvmem_register(config);
>>>> +    if (IS_ERR(nv)) {
>>>> +        devres_free(dr);
>>>> +        return nv;
>>>> +    }
>>>> +
>>>> +    *dr = nv;
>>>> +    devres_add(config->dev, dr);
>>>> +
>>>> +    return nv;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(devm_nvmem_register);
>>>> +
>>>
>>
>