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@xxxxxxxxxxxxxx>
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;
> }
>
