Re: [PATCH] hwmon: sht15: Fix wrong assumptions in device remove callback

[Guenter Roeck <linux@roeck-us.net>]

On Mon, Jul 25, 2022 at 09:43:44PM +0200, Uwe Kleine-König wrote:
> Taking a lock at the beginning of .remove() doesn't prevent new readers.
> With the existing approach it can happen, that a read occurs just when
> the lock was taken blocking the reader until the lock is released at the
> end of the remove callback which then accessed *data that is already
> freed then.
> 
> To actually fix this problem the hwmon core needs some adaption. Until
> this is implemented take the optimistic approach of assuming that all
> readers are gone after hwmon_device_unregister() and
> sysfs_remove_group() as most other drivers do. (And once the core
> implements that, taking the lock would deadlock.)
> 
> So drop the lock, move the reset to after device unregistration to keep
> the device in a workable state until it's deregistered. Also add a error
> message in case the reset fails and return 0 anyhow. (Returning an error
> code, doesn't stop the platform device unregistration and only results
> in a little helpful error message before the devm cleanup handlers are
> called.)
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied.

Thanks,
Guenter

> ---
> Hello,
> 
> the motivation for this patch is to fix the driver to not return an
> error code in .remove(). The long term goal is to make remove callbacks
> return void as returning an error is nearly always wrong and doesn't
> have the effect that driver authors think it has. This patch is a
> preparation for this conversion.
> 
> Best regards
> Uwe
> 
>  drivers/hwmon/sht15.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/sht15.c b/drivers/hwmon/sht15.c
> index 7f4a63959730..ae4d14257a11 100644
> --- a/drivers/hwmon/sht15.c
> +++ b/drivers/hwmon/sht15.c
> @@ -1020,25 +1020,20 @@ static int sht15_probe(struct platform_device *pdev)
>  static int sht15_remove(struct platform_device *pdev)
>  {
>  	struct sht15_data *data = platform_get_drvdata(pdev);
> +	int ret;
>  
> -	/*
> -	 * Make sure any reads from the device are done and
> -	 * prevent new ones beginning
> -	 */
> -	mutex_lock(&data->read_lock);
> -	if (sht15_soft_reset(data)) {
> -		mutex_unlock(&data->read_lock);
> -		return -EFAULT;
> -	}
>  	hwmon_device_unregister(data->hwmon_dev);
>  	sysfs_remove_group(&pdev->dev.kobj, &sht15_attr_group);
> +
> +	ret = sht15_soft_reset(data);
> +	if (ret)
> +		dev_err(&pdev->dev, "Failed to reset device (%pe)\n", ERR_PTR(ret));
> +
>  	if (!IS_ERR(data->reg)) {
>  		regulator_unregister_notifier(data->reg, &data->nb);
>  		regulator_disable(data->reg);
>  	}
>  
> -	mutex_unlock(&data->read_lock);
> -
>  	return 0;
>  }
>