diff mbox series

HID: ensure timely release of driver-allocated resources

Message ID ZFWarGkRAfPOmI6E@google.com (mailing list archive)
State New, archived
Headers show
Series HID: ensure timely release of driver-allocated resources | expand

Commit Message

Dmitry Torokhov May 6, 2023, 12:09 a.m. UTC
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@chromium.org>
Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/hid-core.c | 55 ++++++++++++++++++++++++++++--------------
 include/linux/hid.h    |  1 +
 2 files changed, 38 insertions(+), 18 deletions(-)

Comments

Dmitry Torokhov May 6, 2023, 6:38 p.m. UTC | #1
On Fri, May 05, 2023 at 05:09:16PM -0700, Dmitry Torokhov wrote:
> +	if (down_interruptible(&hdev->driver_input_lock))
> +		return -EINTR;
> +
> +	 ret = __hid_device_probe(hdev);

There is an extra space snuck in before "ret", let me know if you want
me to repost it (or if there are bigger concerns).

Thanks.
Dmitry Torokhov May 22, 2023, 11:30 p.m. UTC | #2
On Sat, May 06, 2023 at 11:38:03AM -0700, Dmitry Torokhov wrote:
> On Fri, May 05, 2023 at 05:09:16PM -0700, Dmitry Torokhov wrote:
> > +	if (down_interruptible(&hdev->driver_input_lock))
> > +		return -EINTR;
> > +
> > +	 ret = __hid_device_probe(hdev);
> 
> There is an extra space snuck in before "ret", let me know if you want
> me to repost it (or if there are bigger concerns).

Gentle ping on this one...
Jiri Kosina May 23, 2023, 1:57 p.m. UTC | #3
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@chromium.org>
> Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  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,
Dmitry Torokhov May 23, 2023, 9:05 p.m. UTC | #4
On Tue, May 23, 2023 at 03:57:03PM +0200, Jiri Kosina wrote:
> 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@chromium.org>
> > Link: https://lore.kernel.org/r/20230505232417.1377393-1-swboyd@chromium.org
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  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).

My bad, I had some patches in my tree that I forgot about. I sent out
a v2.

Thanks.
diff mbox series

Patch

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;
-	}
+	if (!hid_device_check_match(hdev, hdrv, &id))
+		return -ENODEV;
 
 	/* Reset the quirks that have been previously set */
 	hdev->quirks = hid_lookup_quirk(hdev);
 	hdev->driver = hdrv;
 
+	hdev->devres_group_id = devres_open_group(&hdev->dev, NULL, GFP_KERNEL);
+	if (!hdev->devres_group_id)
+		return -ENOMEM;
+
 	if (hdrv->probe) {
 		ret = hdrv->probe(hdev, id);
 	} else { /* default probe */
@@ -2639,15 +2633,36 @@  static int hid_device_probe(struct device *dev)
 			ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	}
 
+	/*
+	 * Note that we are not closing the devres group opened above so
+	 * even resources that were attached to the device after probe is
+	 * run are released when hid_device_remove() is executed. This is
+	 * needed as some drivers would allocate additional resources,
+	 * for example when updating firmware.
+	 */
+
 	if (ret) {
+		devres_release_group(&hdev->dev, hdev->devres_group_id);
 		hid_close_report(hdev);
 		hdev->driver = NULL;
 	}
 
-unlock:
+	return ret;
+}
+
+static int hid_device_probe(struct device *dev)
+{
+	struct hid_device *hdev = to_hid_device(dev);
+	int ret;
+
+	if (down_interruptible(&hdev->driver_input_lock))
+		return -EINTR;
+
+	 ret = __hid_device_probe(hdev);
+
 	if (!hdev->io_started)
 		up(&hdev->driver_input_lock);
-end:
+
 	return ret;
 }
 
@@ -2665,6 +2680,10 @@  static void hid_device_remove(struct device *dev)
 			hdrv->remove(hdev);
 		else /* default remove */
 			hid_hw_stop(hdev);
+
+		/* Release all devres resources allocated by the driver */
+		devres_release_group(&hdev->dev, hdev->devres_group_id);
+
 		hid_close_report(hdev);
 		hdev->driver = NULL;
 	}
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 1ea8c7a3570b..4e4cb3873468 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -596,6 +596,7 @@  struct hid_device {							/* device report descriptor */
 	struct semaphore driver_input_lock;				/* protects the current driver */
 	struct device dev;						/* device */
 	struct hid_driver *driver;
+	void *devres_group_id;						/* ID of probe devres group	*/
 
 	const struct hid_ll_driver *ll_driver;
 	struct mutex ll_open_lock;