Re: [PATCH] HID: ensure timely release of driver-allocated resources

[Jiri Kosina]

On Fri, 5 May 2023, Dmitry Torokhov wrote:

> More and more drivers rely on devres to manage their resources, however
> if bus' probe() and release() methods are not trivial and control some
> of resources as well (for example enable or disable clocks, or attach
> device to a power domain), we need to make sure that driver-allocated
> resources are released immediately after driver's remove() method
> returns, and not postponed until driver core gets around to releasing
> resources.
> 
> In case of HID we should not try to close the report and release
> associated memory until after all devres callbacks are executed. To fix
> that we open a new devres group before calling driver's probe() and
> explicitly release it when we return from driver's remove().
> 
> This is similar to what we did for I2C bus in commit 5b5475826c52 ("i2c:
> ensure timely release of driver-allocated resources"). It is tempting to
> try and move this into driver core, but actually doing so is challenging,
> we need to split bus' remove() method into pre- and post-remove methods,
> which would make the logic even less clear.
> 
> Reported-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@xxxxxxxxxxxx
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>  drivers/hid/hid-core.c | 55 ++++++++++++++++++++++++++++--------------
>  include/linux/hid.h    |  1 +
>  2 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index c4ac9081194c..02a43bba9091 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2602,35 +2602,29 @@ static bool hid_device_check_match(struct hid_device *hdev,
>  	return !hid_ignore_special_drivers;
>  }
>  
> -static int hid_device_probe(struct device *dev)
> +static int __hid_device_probe(struct hid_device *hdev)
>  {
> -	struct hid_driver *hdrv = to_hid_driver(dev->driver);
> -	struct hid_device *hdev = to_hid_device(dev);
> +	struct hid_driver *hdrv = to_hid_driver(hdev->dev.driver);
>  	const struct hid_device_id *id;
>  	int ret;
>  
> -	if (down_interruptible(&hdev->driver_input_lock)) {
> -		ret = -EINTR;
> -		goto end;
> -	}
>  	hdev->io_started = false;
> -
>  	clear_bit(ffs(HID_STAT_REPROBED), &hdev->status);
>  
> -	if (hdev->driver) {
> -		ret = 0;
> -		goto unlock;
> -	}
> +	if (hdev->driver)
> +		return 0;
>  
> -	if (!hid_device_check_match(hdev, hdrv, &id)) {
> -		ret = -ENODEV;
> -		goto unlock;
> -	}

Dmitry, which tree is this patch against, please? The code above looks 
different in current tree (and hasn't been touched for a while).

Thanks,

-- 
Jiri Kosina
SUSE Labs