* Re: [PATCH] eeprom: at24: check the return value of nvmem_unregister()
2017-12-28 21:42 ` Bartosz Golaszewski
@ 2017-12-28 23:05 ` Heiner Kallweit
2017-12-29 9:48 ` Johan Hovold
2018-01-02 11:17 ` Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Heiner Kallweit @ 2017-12-28 23:05 UTC (permalink / raw)
To: Bartosz Golaszewski, Johan Hovold
Cc: linux-i2c, Linux Kernel Mailing List, Srinivas Kandagatla
Am 28.12.2017 um 22:42 schrieb Bartosz Golaszewski:
> 2017-12-28 12:28 GMT+01:00 Johan Hovold <johan@kernel.org>:
>> On Wed, Dec 27, 2017 at 03:10:38PM +0100, Bartosz Golaszewski wrote:
>>> This function can fail with -EBUSY, but we don't check its return
>>> value in at24_remove(). Bail-out of remove() if nvmem_unregister()
>>> doesn't succeed.
>>>
>>> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
>>> ---
>>> drivers/misc/eeprom/at24.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index e79833d62284..fb21e1c45115 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -684,11 +684,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>> static int at24_remove(struct i2c_client *client)
>>> {
>>> struct at24_data *at24;
>>> - int i;
>>> + int i, ret;
>>>
>>> at24 = i2c_get_clientdata(client);
>>>
>>> - nvmem_unregister(at24->nvmem);
>>> + ret = nvmem_unregister(at24->nvmem);
>>> + if (ret)
>>> + return ret;
>>
>> I don't this makes much sense as a driver cannot refuse an unbind by
>> returning an errno from remove(). The return value is simply ignored,
>> remove() will never be called again, and you'd leave everything in an
>> inconsistent state.
>>
>
> Cc: Srinivas
>
> Hi Johan,
>
> I blindly assumed that if there's a return value in remove() then
> someone cares about it. In that case all users of nvmem_unregister()
> that check the return value and bail-out of remove() on failure are
> wrong and in the (very unlikely) event that this routine fails, we
> leak all resources.
>
>> It looks like the nvmem code grabs a reference to the owning module
>> in __nvmem_device_get() which would at least prevent a module unload
>> while another driver is using the device. And the (sysfs) userspace
>> interface should be fine as device removal is handled by the kernfs
>> code.
>
> Indeed. I believe we should remove the -EBUSY return case from
> nvmem_register() and just do what gpiolib does - scream loud
> (dev_crit()) when someone forces a module unload or otherwise
> unregisters the device if some cells are still requested. This would
> also allow us to eventually add a devres variant for nvmem_register().
>
Please see also discussion here, it touches the same topic.
https://lkml.org/lkml/2017/6/4/77
Rgds, Heiner
> What do you think Srinivas?
>
> Best regards,
> Bartosz Golaszewski
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] eeprom: at24: check the return value of nvmem_unregister()
2017-12-28 21:42 ` Bartosz Golaszewski
2017-12-28 23:05 ` Heiner Kallweit
@ 2017-12-29 9:48 ` Johan Hovold
2018-01-02 11:17 ` Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Johan Hovold @ 2017-12-29 9:48 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Johan Hovold, linux-i2c, Linux Kernel Mailing List, Srinivas Kandagatla
On Thu, Dec 28, 2017 at 10:42:21PM +0100, Bartosz Golaszewski wrote:
> 2017-12-28 12:28 GMT+01:00 Johan Hovold <johan@kernel.org>:
> > On Wed, Dec 27, 2017 at 03:10:38PM +0100, Bartosz Golaszewski wrote:
> >> This function can fail with -EBUSY, but we don't check its return
> >> value in at24_remove(). Bail-out of remove() if nvmem_unregister()
> >> doesn't succeed.
> >>
> >> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
> >> ---
> >> drivers/misc/eeprom/at24.c | 6 ++++--
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >> index e79833d62284..fb21e1c45115 100644
> >> --- a/drivers/misc/eeprom/at24.c
> >> +++ b/drivers/misc/eeprom/at24.c
> >> @@ -684,11 +684,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >> static int at24_remove(struct i2c_client *client)
> >> {
> >> struct at24_data *at24;
> >> - int i;
> >> + int i, ret;
> >>
> >> at24 = i2c_get_clientdata(client);
> >>
> >> - nvmem_unregister(at24->nvmem);
> >> + ret = nvmem_unregister(at24->nvmem);
> >> + if (ret)
> >> + return ret;
> >
> > I don't this makes much sense as a driver cannot refuse an unbind by
> > returning an errno from remove(). The return value is simply ignored,
> > remove() will never be called again, and you'd leave everything in an
> > inconsistent state.
> >
>
> Cc: Srinivas
>
> Hi Johan,
>
> I blindly assumed that if there's a return value in remove() then
> someone cares about it. In that case all users of nvmem_unregister()
> that check the return value and bail-out of remove() on failure are
> wrong and in the (very unlikely) event that this routine fails, we
> leak all resources.
I see only one other driver that bails out on deregistration errors
(lpc18xx_eeprom.c), even if other drivers do indeed propagate errors.
> > It looks like the nvmem code grabs a reference to the owning module
> > in __nvmem_device_get() which would at least prevent a module unload
> > while another driver is using the device. And the (sysfs) userspace
> > interface should be fine as device removal is handled by the kernfs
> > code.
>
> Indeed. I believe we should remove the -EBUSY return case from
> nvmem_register() and just do what gpiolib does - scream loud
> (dev_crit()) when someone forces a module unload or otherwise
> unregisters the device if some cells are still requested. This would
> also allow us to eventually add a devres variant for nvmem_register().
I really don't like using devres for deregistration since typically
you'd need a follow-on deallocation step or you end up with a weird
asymmetric interface, but that's another story.
And again, the module unload case would not be a problem, at least when
the device is looked up from device tree, as nvmem then grabs a module
reference.
Johan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] eeprom: at24: check the return value of nvmem_unregister()
2017-12-28 21:42 ` Bartosz Golaszewski
2017-12-28 23:05 ` Heiner Kallweit
2017-12-29 9:48 ` Johan Hovold
@ 2018-01-02 11:17 ` Srinivas Kandagatla
2 siblings, 0 replies; 6+ messages in thread
From: Srinivas Kandagatla @ 2018-01-02 11:17 UTC (permalink / raw)
To: Bartosz Golaszewski, Johan Hovold; +Cc: linux-i2c, Linux Kernel Mailing List
Thanks Bartosz fo adding me in to the loop!!
On 28/12/17 21:42, Bartosz Golaszewski wrote:
> 2017-12-28 12:28 GMT+01:00 Johan Hovold <johan@kernel.org>:
>> On Wed, Dec 27, 2017 at 03:10:38PM +0100, Bartosz Golaszewski wrote:
>>> This function can fail with -EBUSY, but we don't check its return
>>> value in at24_remove(). Bail-out of remove() if nvmem_unregister()
>>> doesn't succeed.
>>>
>>> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
>>> ---
>>> drivers/misc/eeprom/at24.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index e79833d62284..fb21e1c45115 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -684,11 +684,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>> static int at24_remove(struct i2c_client *client)
>>> {
>>> struct at24_data *at24;
>>> - int i;
>>> + int i, ret;
>>>
>>> at24 = i2c_get_clientdata(client);
>>>
>>> - nvmem_unregister(at24->nvmem);
>>> + ret = nvmem_unregister(at24->nvmem);
>>> + if (ret)
>>> + return ret;
>>
>> I don't this makes much sense as a driver cannot refuse an unbind by
>> returning an errno from remove(). The return value is simply ignored,
>> remove() will never be called again, and you'd leave everything in an
>> inconsistent state.
>>
>
> Cc: Srinivas
>
> Hi Johan,
>
> I blindly assumed that if there's a return value in remove() then
> someone cares about it. In that case all users of nvmem_unregister()
> that check the return value and bail-out of remove() on failure are
> wrong and in the (very unlikely) event that this routine fails, we
> leak all resources.
>
Yes, you are correct, returning error from driver remove is issue, as
driver core ignores returns from remove().
Just to make it clear on some assumptions made in
nvmem_register/unregister(). nvmem providers could be part of the other
drivers which can dynamically register and unregister nvmem providers,
so it's not totally necessary for it to be associated with its own
device driver.
>> It looks like the nvmem code grabs a reference to the owning module
>> in __nvmem_device_get() which would at least prevent a module unload
>> while another driver is using the device. And the (sysfs) userspace
>> interface should be fine as device removal is handled by the kernfs
>> code.
>
> Indeed. I believe we should remove the -EBUSY return case from
> nvmem_register() and just do what gpiolib does - scream loud
I think you meant nvmem_unregister().
I don't agree with removing -EBUSY from core, it should be rather
handled by the provider drivers which can do dev_crit or something
similar to shout loud!
> (dev_crit()) when someone forces a module unload or otherwise
> unregisters the device if some cells are still requested. This would
> also allow us to eventually add a devres variant for nvmem_register().
There is already a patchset [1] from Andrey which seems to be what you need.
[1]: https://lkml.org/lkml/2018/1/1/522
Thanks,
Srini
>
> What do you think Srinivas?
>
> Best regards,
> Bartosz Golaszewski
>
^ permalink raw reply [flat|nested] 6+ messages in thread