| Message ID | 20171227141038.31646-1-brgl@bgdev.pl |
|---|---|
| State | Rejected |
| Delegated to: | Bartosz Golaszewski |
| Headers | show |
| Series | eeprom: at24: check the return value of nvmem_unregister() | expand |
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. 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. > for (i = 1; i < at24->num_addresses; i++) > i2c_unregister_device(at24->client[i].client); Johan
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(). What do you think Srinivas? Best regards, Bartosz Golaszewski
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 >
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
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 >
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; for (i = 1; i < at24->num_addresses; i++) i2c_unregister_device(at24->client[i].client);
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(-)