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 <[email protected]>
---
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
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 <[email protected]>
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
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
On Mon, Jan 23, 2023 at 7:18 PM Andy Shevchenko
<[email protected]> 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?
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
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
> <[email protected]> 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
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