eeprom: at24: check the return value of nvmem_unregister()

PATCH In review Bartosz Golaszewski 4 participants 6 messages Dec 27, 2017 – Jan 2, 2018

Subsystems (2)

Char and misc drivers I2C subsystem host drivers

lore

Share & tools

lore

Bartosz Golaszewski

author

2017-12-27 14:10 UTC #0 lore raw

PATCH 1/1 eeprom: at24: check the return value of nvmem_unregister()

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 <[email protected]>

---
 drivers/misc/eeprom/at24.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/misc/eeprom/at24.c +4 -2

index e79833d62284..fb21e1c45115 100644
@@ -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);

Signature · 1 line

2.15.1

Johan Hovold

2017-12-28 11:28 UTC #1 lore raw

Quoted 16 lines · Bartosz Golaszewski

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 <[email protected]>
---
 drivers/misc/eeprom/at24.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +0 -0
index e79833d62284..fb21e1c45115 100644
@@ -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)
 {

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 693

Quoted 10 lines

drivers/misc/eeprom/at24.c +4 -2

 	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.

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 696

Quoted 2 lines

drivers/misc/eeprom/at24.c +0 -0

 	for (i = 1; i < at24->num_addresses; i++)
 		i2c_unregister_device(at24->client[i].client);

Johan

Bartosz Golaszewski

author

2017-12-28 21:42 UTC #2 lore raw

2017-12-28 12:28 GMT+01:00 Johan Hovold <[email protected]>:

Quoted 23 lines · Bartosz Golaszewski

Quoted 22 lines
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 <[email protected]>
---
 drivers/misc/eeprom/at24.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +1 -1
index e79833d62284..fb21e1c45115 100644
@@ -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);
 

Quoted 10 lines

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 693
Quoted 4 lines
drivers/misc/eeprom/at24.c +3 -1
-     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.

Quoted 5 lines

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

Heiner Kallweit

2017-12-28 23:05 UTC #3 lore raw

Am 28.12.2017 um 22:42 schrieb Bartosz Golaszewski:

Quoted 51 lines

2017-12-28 12:28 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 23 lines · Bartosz Golaszewski
Quoted 22 lines
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 <[email protected]>
---
 drivers/misc/eeprom/at24.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +1 -1
index e79833d62284..fb21e1c45115 100644
@@ -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);
 
Quoted 10 lines
drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 693
Quoted 4 lines
drivers/misc/eeprom/at24.c +3 -1
-     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.
Quoted 5 lines
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.

Quoted 6 lines

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

Quoted 5 lines

What do you think Srinivas?
Best regards,
Bartosz Golaszewski

Johan Hovold

2017-12-29 09:48 UTC #4 lore raw

In reply to Bartosz Golaszewski

Quoted 37 lines · Bartosz Golaszewski

2017-12-28 12:28 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 23 lines · Bartosz Golaszewski
Quoted 22 lines
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 <[email protected]>
---
 drivers/misc/eeprom/at24.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +1 -1
index e79833d62284..fb21e1c45115 100644
@@ -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);
 
Quoted 10 lines
drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 693
Quoted 4 lines
drivers/misc/eeprom/at24.c +3 -1
-     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

Quoted 7 lines

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.

Quoted 11 lines

Quoted 5 lines
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

Srinivas Kandagatla

2018-01-02 11:17 UTC #5 lore raw

In reply to Bartosz Golaszewski

Thanks Bartosz fo adding me in to the loop!!

Quoted 37 lines · Bartosz Golaszewski

2017-12-28 12:28 GMT+01:00 Johan Hovold <[email protected]>:
Quoted 23 lines · Bartosz Golaszewski
Quoted 22 lines
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 <[email protected]>
---
  drivers/misc/eeprom/at24.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)
drivers/misc/eeprom/at24.c +1 -1
index e79833d62284..fb21e1c45115 100644
@@ -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);
 
Quoted 10 lines
drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) · line 693
Quoted 4 lines
drivers/misc/eeprom/at24.c +3 -1
-     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

Quoted 8 lines

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.

Quoted 8 lines

Quoted 5 lines
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!

Quoted 3 lines

(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

Quoted 6 lines

What do you think Srinivas?
Best regards,
Bartosz Golaszewski