[PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Andy Shevchenko]

acpi_dev_get_first_match_dev() gets ACPI device with the bumped
refcount. The caller must drop it when it's done.

Fix ACPI device refcounting in apple_gmux_backlight_present().

Fixes: 3cf3b7f012f3 ("ACPI: video: Fix Apple GMUX backlight detection")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/video_detect.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 65cec7bb6d96..0ccde0d4c527 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -114,12 +114,14 @@ static bool apple_gmux_backlight_present(void)
 {
 	struct acpi_device *adev;
 	struct device *dev;
+	bool ret;
 
 	adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
 	if (!adev)
 		return false;
 
-	dev = acpi_get_first_physical_node(adev);
+	dev = get_device(acpi_get_first_physical_node(adev));
+	acpi_dev_put(adev);
 	if (!dev)
 		return false;
 
@@ -127,7 +129,9 @@ static bool apple_gmux_backlight_present(void)
 	 * drivers/platform/x86/apple-gmux.c only supports old style
 	 * Apple GMUX with an IO-resource.
 	 */
-	return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
+	ret = pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
+	put_device(dev);
+	return ret;
 }
 
 /* Force to use vendor driver when the ACPI device is known to be
-- 
2.39.0

[PATCH v1 2/3] ACPI: utils: Add acpi_get_first_match_physical_node()

[Andy Shevchenko]

There are drivers that are using a logic that is combined in the offered
acpi_get_first_match_physical_node(). The rationale to have this helper
not only redunction of the lines of code, but improving the robustness
by properly handling the reference counters on the error paths.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/utils.c    | 28 ++++++++++++++++++++++++++++
 include/acpi/acpi_bus.h |  3 +++
 include/linux/acpi.h    |  6 ++++++
 3 files changed, 37 insertions(+)

diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 2ea14648a661..052e263d2246 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -965,6 +965,34 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
 }
 EXPORT_SYMBOL(acpi_dev_get_first_match_dev);
 
+/**
+ * acpi_get_first_match_physical_node - Return the physical node of the first match of ACPI device
+ * @hid: Hardware ID of the device.
+ * @uid: Unique ID of the device, pass NULL to not check _UID
+ * @hrv: Hardware Revision of the device, pass -1 to not check _HRV
+ *
+ * Return the physical node of the first match of ACPI device if a matching
+ * device was present at the moment of invocation, or NULL otherwise.
+ *
+ * The caller is responsible for invoking put_device() on the returned device.
+ *
+ * See additional information in acpi_dev_present() as well.
+ */
+struct device *acpi_get_first_match_physical_node(const char *hid, const char *uid, s64 hrv)
+{
+	struct acpi_device *adev;
+	struct device *dev;
+
+	adev = acpi_dev_get_first_match_dev(hid, uid, hrv);
+	if (!adev)
+		return NULL;
+
+	dev = get_device(acpi_get_first_physical_node(adev));
+	acpi_dev_put(adev);
+	return dev;
+}
+EXPORT_SYMBOL(acpi_get_first_match_physical_node);
+
 /**
  * acpi_reduced_hardware - Return if this is an ACPI-reduced-hw machine
  *
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 0584e9f6e339..e62af2f71362 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -777,6 +777,9 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv);
 	     adev;							\
 	     adev = acpi_dev_get_next_match_dev(adev, hid, uid, hrv))
 
+struct device *
+acpi_get_first_match_physical_node(const char *hid, const char *uid, s64 hrv);
+
 static inline struct acpi_device *acpi_dev_get(struct acpi_device *adev)
 {
 	return adev ? to_acpi_device(get_device(&adev->dev)) : NULL;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 4b12dad5a8a4..29bae77d819a 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -814,6 +814,12 @@ acpi_dev_get_first_match_dev(const char *hid, const char *uid, s64 hrv)
 	return NULL;
 }
 
+static inline struct device *
+acpi_get_first_match_physical_node(const char *hid, const char *uid, s64 hrv)
+{
+	return NULL;
+}
+
 static inline bool acpi_reduced_hardware(void)
 {
 	return false;
-- 
2.39.0

[PATCH v1 3/3] ACPI: video: Switch to use acpi_get_first_match_physical_node()

[Andy Shevchenko]

Refactor the code to use acpi_get_first_match_physical_node(),
which is currently open coded.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/video_detect.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index 0ccde0d4c527..b23773347e67 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -112,16 +112,10 @@ static bool nvidia_wmi_ec_supported(void)
 
 static bool apple_gmux_backlight_present(void)
 {
-	struct acpi_device *adev;
 	struct device *dev;
 	bool ret;
 
-	adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
-	if (!adev)
-		return false;
-
-	dev = get_device(acpi_get_first_physical_node(adev));
-	acpi_dev_put(adev);
+	dev = acpi_get_first_match_physical_node(GMUX_ACPI_HID, NULL, -1);
 	if (!dev)
 		return false;
 
-- 
2.39.0

Re: [PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Hans de Goede]

Hi Andy,

On 1/23/23 18:10, Andy Shevchenko wrote:
> acpi_dev_get_first_match_dev() gets ACPI device with the bumped
> refcount. The caller must drop it when it's done.
> 
> Fix ACPI device refcounting in apple_gmux_backlight_present().
> 
> Fixes: 3cf3b7f012f3 ("ACPI: video: Fix Apple GMUX backlight detection")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you for your work on this, much appreciated and I like
the new acpi_get_first_match_physical_node().

But I don't think this patch is a good idea. There is a
regression related to apple_gmux_backlight_present()
with a patch-set fixing it pending.

And that patch-set actually removes this function. Adding
a fix for this real, but not really important leak now,
will just make backporting the actual fix harder.

So I would prefer for this patch to not go in and to
go for (a to be submitted v2) of the patch-set fixing
the regression right away instead.

Regards,

Hans



> ---
>  drivers/acpi/video_detect.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index 65cec7bb6d96..0ccde0d4c527 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -114,12 +114,14 @@ static bool apple_gmux_backlight_present(void)
>  {
>  	struct acpi_device *adev;
>  	struct device *dev;
> +	bool ret;
>  
>  	adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1);
>  	if (!adev)
>  		return false;
>  
> -	dev = acpi_get_first_physical_node(adev);
> +	dev = get_device(acpi_get_first_physical_node(adev));
> +	acpi_dev_put(adev);
>  	if (!dev)
>  		return false;
>  
> @@ -127,7 +129,9 @@ static bool apple_gmux_backlight_present(void)
>  	 * drivers/platform/x86/apple-gmux.c only supports old style
>  	 * Apple GMUX with an IO-resource.
>  	 */
> -	return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
> +	ret = pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL;
> +	put_device(dev);
> +	return ret;
>  }
>  
>  /* Force to use vendor driver when the ACPI device is known to be

Re: [PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 06:46:44PM +0100, Hans de Goede wrote:
> On 1/23/23 18:10, Andy Shevchenko wrote:
> > acpi_dev_get_first_match_dev() gets ACPI device with the bumped
> > refcount. The caller must drop it when it's done.
> > 
> > Fix ACPI device refcounting in apple_gmux_backlight_present().

...

> Thank you for your work on this, much appreciated and I like
> the new acpi_get_first_match_physical_node().
> 
> But I don't think this patch is a good idea. There is a
> regression related to apple_gmux_backlight_present()
> with a patch-set fixing it pending.
> 
> And that patch-set actually removes this function. Adding
> a fix for this real, but not really important leak now,
> will just make backporting the actual fix harder.
> 
> So I would prefer for this patch to not go in and to
> go for (a to be submitted v2) of the patch-set fixing
> the regression right away instead.

Maybe I missed something, but I noticed that you actually moved (not killed)
the code which is currently in this function. If it's the case, I prefer my
fix to be imported first.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Rafael J. Wysocki]

On Mon, Jan 23, 2023 at 7:18 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Jan 23, 2023 at 06:46:44PM +0100, Hans de Goede wrote:
> > On 1/23/23 18:10, Andy Shevchenko wrote:
> > > acpi_dev_get_first_match_dev() gets ACPI device with the bumped
> > > refcount. The caller must drop it when it's done.
> > >
> > > Fix ACPI device refcounting in apple_gmux_backlight_present().
>
> ...
>
> > Thank you for your work on this, much appreciated and I like
> > the new acpi_get_first_match_physical_node().
> >
> > But I don't think this patch is a good idea. There is a
> > regression related to apple_gmux_backlight_present()
> > with a patch-set fixing it pending.
> >
> > And that patch-set actually removes this function. Adding
> > a fix for this real, but not really important leak now,
> > will just make backporting the actual fix harder.
> >
> > So I would prefer for this patch to not go in and to
> > go for (a to be submitted v2) of the patch-set fixing
> > the regression right away instead.
>
> Maybe I missed something, but I noticed that you actually moved (not killed)
> the code which is currently in this function. If it's the case, I prefer my
> fix to be imported first.

Well, what about making the new code not leak?

That way the separate fix won't be necessary any more, will it?

Re: [PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Hans de Goede]

Hi,

On 1/23/23 19:18, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 06:46:44PM +0100, Hans de Goede wrote:
>> On 1/23/23 18:10, Andy Shevchenko wrote:
>>> acpi_dev_get_first_match_dev() gets ACPI device with the bumped
>>> refcount. The caller must drop it when it's done.
>>>
>>> Fix ACPI device refcounting in apple_gmux_backlight_present().
> 
> ...
> 
>> Thank you for your work on this, much appreciated and I like
>> the new acpi_get_first_match_physical_node().
>>
>> But I don't think this patch is a good idea. There is a
>> regression related to apple_gmux_backlight_present()
>> with a patch-set fixing it pending.
>>
>> And that patch-set actually removes this function. Adding
>> a fix for this real, but not really important leak now,
>> will just make backporting the actual fix harder.
>>
>> So I would prefer for this patch to not go in and to
>> go for (a to be submitted v2) of the patch-set fixing
>> the regression right away instead.
> 
> Maybe I missed something, but I noticed that you actually moved (not killed)
> the code which is currently in this function. If it's the case, I prefer my
> fix to be imported first.

The code is not really moved, patch 2/3 of my patch-set factors out
the detection code from drivers/platform/x86/apple-gmux.c's probe
function. The new factored out code uses a similar construct as
the apple_gmux_backlight_present() code (including the same leak).

Then patch 3/3 drops apple_gmux_backlight_present() and calls
the new factored out probe code.

I'll fix the leak in v2 and then add the 3 patches to pdx86/fixes
for the next pull-req to Linus (thus also fixing the leak).

Regards,

Hans

Re: [PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 08:24:27PM +0100, Rafael J. Wysocki wrote:
> On Mon, Jan 23, 2023 at 7:18 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Jan 23, 2023 at 06:46:44PM +0100, Hans de Goede wrote:
> > > On 1/23/23 18:10, Andy Shevchenko wrote:
> > > > acpi_dev_get_first_match_dev() gets ACPI device with the bumped
> > > > refcount. The caller must drop it when it's done.
> > > >
> > > > Fix ACPI device refcounting in apple_gmux_backlight_present().

...

> > > Thank you for your work on this, much appreciated and I like
> > > the new acpi_get_first_match_physical_node().
> > >
> > > But I don't think this patch is a good idea. There is a
> > > regression related to apple_gmux_backlight_present()
> > > with a patch-set fixing it pending.
> > >
> > > And that patch-set actually removes this function. Adding
> > > a fix for this real, but not really important leak now,
> > > will just make backporting the actual fix harder.
> > >
> > > So I would prefer for this patch to not go in and to
> > > go for (a to be submitted v2) of the patch-set fixing
> > > the regression right away instead.
> >
> > Maybe I missed something, but I noticed that you actually moved (not killed)
> > the code which is currently in this function. If it's the case, I prefer my
> > fix to be imported first.
> 
> Well, what about making the new code not leak?
> 
> That way the separate fix won't be necessary any more, will it?

Yes, it will.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present()

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 11:10:26PM +0100, Hans de Goede wrote:
> On 1/23/23 19:18, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 06:46:44PM +0100, Hans de Goede wrote:
> >> On 1/23/23 18:10, Andy Shevchenko wrote:
> >>> acpi_dev_get_first_match_dev() gets ACPI device with the bumped
> >>> refcount. The caller must drop it when it's done.
> >>>
> >>> Fix ACPI device refcounting in apple_gmux_backlight_present().

...

> >> Thank you for your work on this, much appreciated and I like
> >> the new acpi_get_first_match_physical_node().
> >>
> >> But I don't think this patch is a good idea. There is a
> >> regression related to apple_gmux_backlight_present()
> >> with a patch-set fixing it pending.
> >>
> >> And that patch-set actually removes this function. Adding
> >> a fix for this real, but not really important leak now,
> >> will just make backporting the actual fix harder.
> >>
> >> So I would prefer for this patch to not go in and to
> >> go for (a to be submitted v2) of the patch-set fixing
> >> the regression right away instead.
> > 
> > Maybe I missed something, but I noticed that you actually moved (not killed)
> > the code which is currently in this function. If it's the case, I prefer my
> > fix to be imported first.
> 
> The code is not really moved, patch 2/3 of my patch-set factors out
> the detection code from drivers/platform/x86/apple-gmux.c's probe
> function. The new factored out code uses a similar construct as
> the apple_gmux_backlight_present() code (including the same leak).
> 
> Then patch 3/3 drops apple_gmux_backlight_present() and calls
> the new factored out probe code.
> 
> I'll fix the leak in v2 and then add the 3 patches to pdx86/fixes
> for the next pull-req to Linus (thus also fixing the leak).

As agreed with Rafael, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] ACPI: utils: Add acpi_get_first_match_physical_node()

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 07:10:05PM +0200, Andy Shevchenko wrote:
> There are drivers that are using a logic that is combined in the offered
> acpi_get_first_match_physical_node(). The rationale to have this helper
> not only redunction of the lines of code, but improving the robustness
> by properly handling the reference counters on the error paths.

After rebasing on top of the latest code base (with Hans' patches included) I
checked the users of similar code flow and found that there is no sense to
provide this helper now. It's only one user for this API as is, otherwise it
needs an access to struct acpi_device, which the proposed API doesn't provide.

Hence, self-NAK.

-- 
With Best Regards,
Andy Shevchenko