Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 23 May 2023 14:05:59 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Jiri Kosina <jikos@kernel.org>
Cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Stephen Boyd <swboyd@chromium.org>,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: ensure timely release of driver-allocated resources
Message-ID: <ZG0qt0ji5dgJiDpT@google.com>
References: <ZFWarGkRAfPOmI6E@google.com>
 <nycvar.YFH.7.76.2305231554250.29760@cbobk.fhfr.pm>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <nycvar.YFH.7.76.2305231554250.29760@cbobk.fhfr.pm>
Precedence: bulk

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.

-- 
Dmitry