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

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

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@gmail.com>
>>> ---
>>>   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.
> 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.
> 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);
>>> +
>>
>