| Message ID | 20180712172332.61399-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] ACPI / utils: Introduce acpi_dev_get_first_device() helper | expand |
Hi, On 12-07-18 19:23, Andy Shevchenko wrote: > acpi_dev_present() and acpi_dev_get_first_match_name() are missing > put_device() call and thus keeping reference counting unbalanced. > > In order to fix the issue introduce a new helper to convert existing users > one-by-one to a better API. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/acpi/utils.c | 50 ++++++++++++++++++++++++----------------- > include/acpi/acpi_bus.h | 2 ++ > include/linux/acpi.h | 6 +++++ > 3 files changed, 38 insertions(+), 20 deletions(-) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 78db97687f26..b54651b3d4bd 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -738,7 +738,6 @@ bool acpi_dev_found(const char *hid) > EXPORT_SYMBOL(acpi_dev_found); > > struct acpi_dev_match_info { > - const char *dev_name; > struct acpi_device_id hid[2]; > const char *uid; > s64 hrv; > @@ -758,8 +757,6 @@ static int acpi_dev_match_cb(struct device *dev, void *data) > strcmp(adev->pnp.unique_id, match->uid))) > return 0; > > - match->dev_name = acpi_dev_name(adev); > - > if (match->hrv == -1) > return 1; > > @@ -771,18 +768,18 @@ static int acpi_dev_match_cb(struct device *dev, void *data) > } > > /** > - * acpi_dev_present - Detect that a given ACPI device is present > + * acpi_dev_get_first_match - Return a first match of ACPI device if present > * @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 %true if a matching device was present at the moment of invocation. > - * Note that if the device is pluggable, it may since have disappeared. > + * Return a pointer to the first matching ACPI device. > + * Caller must put device back to balance reference counting. > * > * Note that unlike acpi_dev_found() this function checks the status > - * of the device. So for devices which are present in the dsdt, but > + * of the device. So for devices which are present in the DSDT, but > * which are disabled (their _STA callback returns 0) this function > - * will return false. > + * will return NULL. > * > * For this function to work, acpi_bus_scan() must have been executed > * which happens in the subsys_initcall() subsection. Hence, do not > @@ -790,7 +787,8 @@ static int acpi_dev_match_cb(struct device *dev, void *data) > * instead). Calling from module_init() is fine (which is synonymous > * with device_initcall()). > */ > -bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > +struct acpi_device * > +acpi_dev_get_first_match(const char *hid, const char *uid, s64 hrv) > { > struct acpi_dev_match_info match = {}; > struct device *dev; > @@ -800,7 +798,25 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > match.hrv = hrv; > > dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb); > - return !!dev; > + return dev ? to_acpi_device(dev) : NULL; > +} > +EXPORT_SYMBOL(acpi_dev_get_first_match); > + > +/** > + * acpi_dev_present - Detect that a given ACPI device is present > + * @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 > + * > + * DEPRECATED, use acpi_dev_get_first_match() directly! > + * > + * Return %true if a matching device is present. > + */ > +bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > +{ > + struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, hrv); > + > + return !!adev; > } > EXPORT_SYMBOL(acpi_dev_present); Why not just do: bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) { struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, hrv); if (!adev) return false; put_device(&adev->dev); return true; } And not deprecate this. This fixes the leak while keeping the API usage simple for users of this API. Having to do a put_device in all callers seems cumbersome if we can just do it here. Regards, Hans > > @@ -810,23 +826,17 @@ EXPORT_SYMBOL(acpi_dev_present); > * @uid: Unique ID of the device, pass NULL to not check _UID > * @hrv: Hardware Revision of the device, pass -1 to not check _HRV > * > + * DEPRECATED, use acpi_dev_get_first_match() directly! > + * > * Return device name if a matching device was present > * at the moment of invocation, or NULL otherwise. > - * > - * See additional information in acpi_dev_present() as well. > */ > const char * > acpi_dev_get_first_match_name(const char *hid, const char *uid, s64 hrv) > { > - struct acpi_dev_match_info match = {}; > - struct device *dev; > - > - strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); > - match.uid = uid; > - match.hrv = hrv; > + struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, hrv); > > - dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb); > - return dev ? match.dev_name : NULL; > + return adev ? acpi_dev_name(adev) : NULL; > } > EXPORT_SYMBOL(acpi_dev_get_first_match_name); > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index ba4dd54f2c82..53ca4403f772 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -91,6 +91,8 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev, > bool acpi_dev_found(const char *hid); > bool acpi_dev_present(const char *hid, const char *uid, s64 hrv); > > +struct acpi_device * > +acpi_dev_get_first_match(const char *hid, const char *uid, s64 hrv); > const char * > acpi_dev_get_first_match_name(const char *hid, const char *uid, s64 hrv); > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index e54f40974eb0..098e0af003b4 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -657,6 +657,12 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > return false; > } > > +struct acpi_device * > +acpi_dev_get_first_match(const char *hid, const char *uid, s64 hrv) > +{ > + return NULL; > +} > + > static inline const char * > acpi_dev_get_first_match_name(const char *hid, const char *uid, s64 hrv) > { > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2018-07-12 at 21:20 +0200, Hans de Goede wrote: > Hi, > > On 12-07-18 19:23, Andy Shevchenko wrote: > > acpi_dev_present() and acpi_dev_get_first_match_name() are missing > > put_device() call and thus keeping reference counting unbalanced. > > > > In order to fix the issue introduce a new helper to convert existing > > users > > one-by-one to a better API. > > > > > > +bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > > +{ > > + struct acpi_device *adev = acpi_dev_get_first_match(hid, > > uid, hrv); > > + > > + return !!adev; > > } > > EXPORT_SYMBOL(acpi_dev_present); > > Why not just do: > > bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > { > struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, > hrv); > > if (!adev) > return false; > > put_device(&adev->dev); > return true; > } > > And not deprecate this. This fixes the leak while keeping the > API usage simple for users of this API. Having to do a put_device > in all callers seems cumbersome if we can just do it here. The new API has been dictated by the nature of bus_find_device(). Thus same rules applies. However, in this very particular case (since we are a) just checking for presense, and b) don't care if device will gone) it's okay like you proposed. So, would you agree on splitting this to several changes for better granularity, i.e. - introduce new API - convert acpi_dev_present() to use it - fix the issue (squash with previous?) - ... do similar to acpi_dev_get_first_match_name() ... ? Of course, we can fix acpi_dev_present() right now w/o new API, but it feels to be half baked without fixing acpi_dev_get_first_match_name().
Hi Andy,
I love your patch! Yet something to improve:
[auto build test ERROR on pm/linux-next]
[also build test ERROR on v4.18-rc4 next-20180713]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Andy-Shevchenko/ACPI-utils-Introduce-acpi_dev_get_first_device-helper/20180714-120815
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
arch/x86/kernel/setup.o: In function `acpi_dev_get_first_match':
>> setup.c:(.text+0x3): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
arch/x86/kernel/i8259.o: In function `acpi_dev_get_first_match':
i8259.c:(.text+0x2c2): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
arch/x86/kernel/irqinit.o: In function `acpi_dev_get_first_match':
irqinit.c:(.text+0x0): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
arch/x86/kernel/bootflag.o: In function `acpi_dev_get_first_match':
bootflag.c:(.text+0x0): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
arch/x86/kernel/e820.o: In function `acpi_dev_get_first_match':
e820.c:(.text+0xb1): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
arch/x86/kernel/pci-dma.o: In function `acpi_dev_get_first_match':
pci-dma.c:(.text+0x0): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
arch/x86/kernel/rtc.o: In function `acpi_dev_get_first_match':
rtc.c:(.text+0x41): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
kernel/sysctl.o: In function `acpi_dev_get_first_match':
sysctl.c:(.text+0x0): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
kernel/dma/mapping.o: In function `acpi_dev_get_first_match':
mapping.c:(.text+0xfe): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
drivers/base/platform.o: In function `acpi_dev_get_first_match':
platform.c:(.text+0x1eb): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
drivers/base/cpu.o: In function `acpi_dev_get_first_match':
cpu.c:(.text+0x1a1): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
drivers/base/property.o: In function `acpi_dev_get_first_match':
property.c:(.text+0x2d3): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
drivers/base/cacheinfo.o: In function `acpi_dev_get_first_match':
cacheinfo.c:(.text+0x2e5): multiple definition of `acpi_dev_get_first_match'
init/main.o:main.c:(.text+0x19): first defined here
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi, On 13-07-18 14:36, Andy Shevchenko wrote: > On Thu, 2018-07-12 at 21:20 +0200, Hans de Goede wrote: >> Hi, >> >> On 12-07-18 19:23, Andy Shevchenko wrote: >>> acpi_dev_present() and acpi_dev_get_first_match_name() are missing >>> put_device() call and thus keeping reference counting unbalanced. >>> >>> In order to fix the issue introduce a new helper to convert existing >>> users >>> one-by-one to a better API. >>> >>> > >>> +bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) >>> +{ >>> + struct acpi_device *adev = acpi_dev_get_first_match(hid, >>> uid, hrv); >>> + >>> + return !!adev; >>> } >>> EXPORT_SYMBOL(acpi_dev_present); >> >> Why not just do: >> >> bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) >> { >> struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, >> hrv); >> >> if (!adev) >> return false; >> >> put_device(&adev->dev); >> return true; >> } >> >> And not deprecate this. This fixes the leak while keeping the >> API usage simple for users of this API. Having to do a put_device >> in all callers seems cumbersome if we can just do it here. > > The new API has been dictated by the nature of bus_find_device(). > Thus same rules applies. > > However, in this very particular case (since we are a) just checking for > presense, and b) don't care if device will gone) it's okay like you > proposed. > > So, would you agree on splitting this to several changes for better > granularity, i.e. > > - introduce new API > - convert acpi_dev_present() to use it > - fix the issue (squash with previous?) > - ... do similar to acpi_dev_get_first_match_name() ... > > ? Yes that is fine with me. > Of course, we can fix acpi_dev_present() right now w/o new API, but it > feels to be half baked without fixing acpi_dev_get_first_match_name(). First fixing acpi_dev_get_first_match_name() is fine with me. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c index 78db97687f26..b54651b3d4bd 100644 --- a/drivers/acpi/utils.c +++ b/drivers/acpi/utils.c @@ -738,7 +738,6 @@ bool acpi_dev_found(const char *hid) EXPORT_SYMBOL(acpi_dev_found); struct acpi_dev_match_info { - const char *dev_name; struct acpi_device_id hid[2]; const char *uid; s64 hrv; @@ -758,8 +757,6 @@ static int acpi_dev_match_cb(struct device *dev, void *data) strcmp(adev->pnp.unique_id, match->uid))) return 0; - match->dev_name = acpi_dev_name(adev); - if (match->hrv == -1) return 1; @@ -771,18 +768,18 @@ static int acpi_dev_match_cb(struct device *dev, void *data) } /** - * acpi_dev_present - Detect that a given ACPI device is present + * acpi_dev_get_first_match - Return a first match of ACPI device if present * @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 %true if a matching device was present at the moment of invocation. - * Note that if the device is pluggable, it may since have disappeared. + * Return a pointer to the first matching ACPI device. + * Caller must put device back to balance reference counting. * * Note that unlike acpi_dev_found() this function checks the status - * of the device. So for devices which are present in the dsdt, but + * of the device. So for devices which are present in the DSDT, but * which are disabled (their _STA callback returns 0) this function - * will return false. + * will return NULL. * * For this function to work, acpi_bus_scan() must have been executed * which happens in the subsys_initcall() subsection. Hence, do not @@ -790,7 +787,8 @@ static int acpi_dev_match_cb(struct device *dev, void *data) * instead). Calling from module_init() is fine (which is synonymous * with device_initcall()). */ -bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) +struct acpi_device * +acpi_dev_get_first_match(const char *hid, const char *uid, s64 hrv) { struct acpi_dev_match_info match = {}; struct device *dev; @@ -800,7 +798,25 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) match.hrv = hrv; dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb); - return !!dev; + return dev ? to_acpi_device(dev) : NULL; +} +EXPORT_SYMBOL(acpi_dev_get_first_match); + +/** + * acpi_dev_present - Detect that a given ACPI device is present + * @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 + * + * DEPRECATED, use acpi_dev_get_first_match() directly! + * + * Return %true if a matching device is present. + */ +bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) +{ + struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, hrv); + + return !!adev; } EXPORT_SYMBOL(acpi_dev_present); @@ -810,23 +826,17 @@ EXPORT_SYMBOL(acpi_dev_present); * @uid: Unique ID of the device, pass NULL to not check _UID * @hrv: Hardware Revision of the device, pass -1 to not check _HRV * + * DEPRECATED, use acpi_dev_get_first_match() directly! + * * Return device name if a matching device was present * at the moment of invocation, or NULL otherwise. - * - * See additional information in acpi_dev_present() as well. */ const char * acpi_dev_get_first_match_name(const char *hid, const char *uid, s64 hrv) { - struct acpi_dev_match_info match = {}; - struct device *dev; - - strlcpy(match.hid[0].id, hid, sizeof(match.hid[0].id)); - match.uid = uid; - match.hrv = hrv; + struct acpi_device *adev = acpi_dev_get_first_match(hid, uid, hrv); - dev = bus_find_device(&acpi_bus_type, NULL, &match, acpi_dev_match_cb); - return dev ? match.dev_name : NULL; + return adev ? acpi_dev_name(adev) : NULL; } EXPORT_SYMBOL(acpi_dev_get_first_match_name); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index ba4dd54f2c82..53ca4403f772 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -91,6 +91,8 @@ acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev, bool acpi_dev_found(const char *hid); bool acpi_dev_present(const char *hid, const char *uid, s64 hrv); +struct acpi_device * +acpi_dev_get_first_match(const char *hid, const char *uid, s64 hrv); const char * acpi_dev_get_first_match_name(const char *hid, const char *uid, s64 hrv); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index e54f40974eb0..098e0af003b4 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -657,6 +657,12 @@ static inline bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) return false; } +struct acpi_device * +acpi_dev_get_first_match(const char *hid, const char *uid, s64 hrv) +{ + return NULL; +} + static inline const char * acpi_dev_get_first_match_name(const char *hid, const char *uid, s64 hrv) {
acpi_dev_present() and acpi_dev_get_first_match_name() are missing put_device() call and thus keeping reference counting unbalanced. In order to fix the issue introduce a new helper to convert existing users one-by-one to a better API. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/acpi/utils.c | 50 ++++++++++++++++++++++++----------------- include/acpi/acpi_bus.h | 2 ++ include/linux/acpi.h | 6 +++++ 3 files changed, 38 insertions(+), 20 deletions(-)