| Message ID | 20230123171006.58274-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v1,1/3] ACPI: video: Fix refcounting in apple_gmux_backlight_present() | expand |
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
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.
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?
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 > <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.
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!
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
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(-)