Re: [PATCH] eeprom: at24: check the return value of nvmem_unregister()

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

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