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

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

[Heiner Kallweit]

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(+)

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);
+}
+
+/**
+ * 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)
+{
+	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);
+
 static struct nvmem_device *__nvmem_device_get(struct device_node *np,
 					       struct nvmem_cell **cellp,
 					       const char *cell_id)
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index cd93416d..4b92066d 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -42,6 +42,7 @@ struct nvmem_config {
 #if IS_ENABLED(CONFIG_NVMEM)
 
 struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
+struct nvmem_device *devm_nvmem_register(const struct nvmem_config *cfg);
 int nvmem_unregister(struct nvmem_device *nvmem);
 
 #else
@@ -51,6 +52,12 @@ static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline struct nvmem_device *
+			devm_nvmem_register(const struct nvmem_config *c)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline int nvmem_unregister(struct nvmem_device *nvmem)
 {
 	return -ENOSYS;
-- 
2.13.0

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

[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?


> +}
> +
> +/**
> + * 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);
> +

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

[Heiner Kallweit]

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.
Checking the refcount I see more as a debug feature and I think making
nvmem_unregister return void plus a WARN() if refcount != 0 would be better.

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);
>> +
> 

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

[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@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);
>>> +
>>
> 

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@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.

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);
>>>> +
>>>
>>
>