| Message ID | 20240613-acpi-sysfs-groups-v1-2-665e0deb052a@weissschuh.net |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: sysfs: manage sysfs attributes through device core | expand |
On Thu, Jun 13, 2024 at 10:15:33PM +0200, Thomas Weißschuh wrote: > The string assigned to dev->pnp.str effectively shares the lifetime of > the device. Use devm_-APIs to avoid a manual cleanup path. > > This will be useful when the attributes themselves will be managed by > the device core. This is in -next since 20240624 and appears to be causing issues on Cavium Thunder X2 in the Arm lab - with arm64 defconfig we see a bunch of log messages like: <6>[ 50.120962] ACPI: button: Power Button [PWRB] <6>[ 50.120962] ACPI: button: Power Button [PWRB] <2>[ 50.138595] acpi LNXTHERM:00: Resources present before probing <2>[ 50.138595] acpi LNXTHERM:00: Resources present before probing <2>[ 50.150873] acpi LNXTHERM:01: Resources present before probing <2>[ 50.150873] acpi LNXTHERM:01: Resources present before probing <2>[ 50.163134] acpi LNXTHERM:02: Resources present before probing <2>[ 50.163134] acpi LNXTHERM:02: Resources present before probing <2>[ 50.175393] acpi LNXTHERM:03: Resources present before probing <2>[ 50.175393] acpi LNXTHERM:03: Resources present before probing <2>[ 50.187653] acpi LNXTHERM:04: Resources present before probing <2>[ 50.187653] acpi LNXTHERM:04: Resources present before probing <2>[ 50.199913] acpi LNXTHERM:05: Resources present before probing <2>[ 50.199913] acpi LNXTHERM:05: Resources present before probing <2>[ 50.212171] acpi LNXTHERM:06: Resources present before probing <2>[ 50.212171] acpi LNXTHERM:06: Resources present before probing <2>[ 50.224433] acpi LNXTHERM:07: Resources present before probing <2>[ 50.224433] acpi LNXTHERM:07: Resources present before probing <2>[ 50.236703] acpi LNXTHERM:08: Resources present before probing (some other bug seems to be doubling all the log lines.) We also see PCI getting upset: <6>[ 50.238119] pcieport 0000:00:03.0: Refused to change power state from D0 to D3hot and the ethernet driver gets confused: [ 71.592299] mlx5_core 0000:0b:00.1: Port module event: module 1, Cable unplugged while the cable is most definitely connected, we're netbooting. A bisect pointed at this commit, full bisect log below: git bisect start # status: waiting for both good and bad commits # bad: [62c97045b8f720c2eac807a5f38e26c9ed512371] Add linux-next specific files for 20240624 git bisect bad 62c97045b8f720c2eac807a5f38e26c9ed512371 # status: waiting for good commit(s), bad commit known # good: [f2661062f16b2de5d7b6a5c42a9a5c96326b8454] Linux 6.10-rc5 git bisect good f2661062f16b2de5d7b6a5c42a9a5c96326b8454 # bad: [dff8aaf6c690e2a3ff1ece04a78d494e7111b97f] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git git bisect bad dff8aaf6c690e2a3ff1ece04a78d494e7111b97f # good: [0dcf65f1a6999ce2efcce2d956c43698ad5cb910] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git git bisect good 0dcf65f1a6999ce2efcce2d956c43698ad5cb910 # bad: [1da968e3bbd7ea713af0a23ff6b708772d024691] Merge branch 'cpufreq/arm/linux-next' of git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git git bisect bad 1da968e3bbd7ea713af0a23ff6b708772d024691 # good: [998d61dc008b677c845a0c7ef69dcb1d2b3d5546] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/printk/linux.git git bisect good 998d61dc008b677c845a0c7ef69dcb1d2b3d5546 # good: [5dc494b479a6e7c4a4425bb353c25b219e07c894] Merge branch 'hwmon-next' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git git bisect good 5dc494b479a6e7c4a4425bb353c25b219e07c894 # good: [156922faabcef2979cb2ddc2fbaa659b5ea37f54] media: atomisp: Switch to new Intel CPU model defines git bisect good 156922faabcef2979cb2ddc2fbaa659b5ea37f54 # good: [2b582118f1f42928129922c94a174f0cc42b38fe] Merge branch 'thermal-intel' into linux-next git bisect good 2b582118f1f42928129922c94a174f0cc42b38fe # good: [d499aee48012b899f5bc814fef9021520411cab7] Merge branch 'docs-mw' into docs-next git bisect good d499aee48012b899f5bc814fef9021520411cab7 # bad: [1c524c86afa8555e8add0c83c9d58e147cdf8f23] Merge branch 'pm-cpufreq' into linux-next git bisect bad 1c524c86afa8555e8add0c83c9d58e147cdf8f23 # good: [70d80903e265dc81e2bf390e5b301a32b2344ff4] Merge branch 'thermal' into linux-next git bisect good 70d80903e265dc81e2bf390e5b301a32b2344ff4 # bad: [fe66d86311693574aca1b9624f92e273c13d1b3b] ACPI: sysfs: remove return value of acpi_device_setup_files() git bisect bad fe66d86311693574aca1b9624f92e273c13d1b3b # bad: [30fb30aa9ab68e0f638ae775de6284c41e8910b2] ACPI: sysfs: use device lifecycle for _STR result git bisect bad 30fb30aa9ab68e0f638ae775de6284c41e8910b2 # good: [3fd84db96b212a321ad381bf0341f45f952285b7] ACPI: sysfs: convert utf-16 from _STR to utf-8 only once git bisect good 3fd84db96b212a321ad381bf0341f45f952285b7 # first bad commit: [30fb30aa9ab68e0f638ae775de6284c41e8910b2] ACPI: sysfs: use device lifecycle for _STR result
On 2024-06-25 21:57:13+0000, Mark Brown wrote: > On Thu, Jun 13, 2024 at 10:15:33PM +0200, Thomas Weißschuh wrote: > > The string assigned to dev->pnp.str effectively shares the lifetime of > > the device. Use devm_-APIs to avoid a manual cleanup path. > > > > This will be useful when the attributes themselves will be managed by > > the device core. > > This is in -next since 20240624 and appears to be causing issues on > Cavium Thunder X2 in the Arm lab - with arm64 defconfig we see a bunch > of log messages like: > > <6>[ 50.120962] ACPI: button: Power Button [PWRB] > <6>[ 50.120962] ACPI: button: Power Button [PWRB] > <2>[ 50.138595] acpi LNXTHERM:00: Resources present before probing > <2>[ 50.138595] acpi LNXTHERM:00: Resources present before probing > <2>[ 50.150873] acpi LNXTHERM:01: Resources present before probing > <2>[ 50.150873] acpi LNXTHERM:01: Resources present before probing > <2>[ 50.163134] acpi LNXTHERM:02: Resources present before probing > <2>[ 50.163134] acpi LNXTHERM:02: Resources present before probing > <2>[ 50.175393] acpi LNXTHERM:03: Resources present before probing > <2>[ 50.175393] acpi LNXTHERM:03: Resources present before probing > <2>[ 50.187653] acpi LNXTHERM:04: Resources present before probing > <2>[ 50.187653] acpi LNXTHERM:04: Resources present before probing > <2>[ 50.199913] acpi LNXTHERM:05: Resources present before probing > <2>[ 50.199913] acpi LNXTHERM:05: Resources present before probing > <2>[ 50.212171] acpi LNXTHERM:06: Resources present before probing > <2>[ 50.212171] acpi LNXTHERM:06: Resources present before probing > <2>[ 50.224433] acpi LNXTHERM:07: Resources present before probing > <2>[ 50.224433] acpi LNXTHERM:07: Resources present before probing > <2>[ 50.236703] acpi LNXTHERM:08: Resources present before probing This does make sense, the device is not yet bound to a driver. Which apparently precludes the usage of devres. Skipping this commit and doing the kfree() inside acpi_device_remove_file() also shouldn't work, as the attributes would live longer than the string. I'm wondering why dev->pnp.str does not share the lifecycle of the rest of dev->pnp, acpi_init_device_object()/acpi_device_release(). Or we evaluate _STR during is_visible(), which keeps the single evaluation, or during description_show() which is the same as all the other attributes. I'm also wondering why the _STR attribute behaved differently in the first place. Does the patch below work better? > (some other bug seems to be doubling all the log lines.) We also see > PCI getting upset: > > <6>[ 50.238119] pcieport 0000:00:03.0: Refused to change power state from D0 to D3hot > > and the ethernet driver gets confused: > > [ 71.592299] mlx5_core 0000:0b:00.1: Port module event: module 1, Cable unplugged > > while the cable is most definitely connected, we're netbooting. A > bisect pointed at this commit, full bisect log below: All these different kinds of errors are bisected to this commit? diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c index c85ec754931c..350915b06472 100644 --- a/drivers/acpi/device_sysfs.c +++ b/drivers/acpi/device_sysfs.c @@ -510,6 +510,40 @@ static struct attribute *acpi_attrs[] = { NULL }; +static const char *acpi_device_str(struct acpi_device *dev) +{ + struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL}; + union acpi_object *str_obj; + acpi_status status; + const char *ret; + char buf[512]; + int result; + + if (!acpi_has_method(dev->handle, "_STR")) + return NULL; + + status = acpi_evaluate_object(dev->handle, "_STR", + NULL, &buffer); + if (ACPI_FAILURE(status)) + return NULL; + + str_obj = buffer.pointer; + /* + * The _STR object contains a Unicode identifier for a device. + * We need to convert to utf-8 so it can be displayed. + */ + result = utf16s_to_utf8s((wchar_t *)str_obj->buffer.pointer, + str_obj->buffer.length, + UTF16_LITTLE_ENDIAN, + buf, sizeof(buf) - 1); + buf[result++] = '\0'; + + ret = kstrdup(buf, GFP_KERNEL); + kfree(buffer.pointer); + + return ret; +} + static bool acpi_show_attr(struct acpi_device *dev, const struct device_attribute *attr) { /* @@ -524,8 +558,11 @@ static bool acpi_show_attr(struct acpi_device *dev, const struct device_attribut /* * If device has _STR, 'description' file is created */ - if (attr == &dev_attr_description) + if (attr == &dev_attr_description) { + kfree(dev->pnp.str); + dev->pnp.str = acpi_device_str(dev); return dev->pnp.str; + } if (attr == &dev_attr_adr) return dev->pnp.type.bus_address; @@ -581,47 +618,12 @@ const struct attribute_group *acpi_groups[] = { NULL }; -static const char *devm_acpi_device_str(struct acpi_device *dev) -{ - struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL}; - union acpi_object *str_obj; - acpi_status status; - const char *ret; - char buf[512]; - int result; - - if (!acpi_has_method(dev->handle, "_STR")) - return NULL; - - status = acpi_evaluate_object(dev->handle, "_STR", - NULL, &buffer); - if (ACPI_FAILURE(status)) - return NULL; - - str_obj = buffer.pointer; - /* - * The _STR object contains a Unicode identifier for a device. - * We need to convert to utf-8 so it can be displayed. - */ - result = utf16s_to_utf8s((wchar_t *)str_obj->buffer.pointer, - str_obj->buffer.length, - UTF16_LITTLE_ENDIAN, - buf, sizeof(buf) - 1); - buf[result++] = '\0'; - - ret = devm_kstrdup(&dev->dev, buf, GFP_KERNEL); - kfree(buffer.pointer); - - return ret; -} - /** * acpi_device_setup_files - Create sysfs attributes of an ACPI device. * @dev: ACPI device object. */ void acpi_device_setup_files(struct acpi_device *dev) { - dev->pnp.str = devm_acpi_device_str(dev); acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data); } diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c index 49a8172fe0de..84c4190f03fb 100644 --- a/drivers/acpi/scan.c +++ b/drivers/acpi/scan.c @@ -1456,6 +1456,7 @@ void acpi_free_pnp_ids(struct acpi_device_pnp *pnp) kfree(id); } kfree(pnp->unique_id); + kfree(pnp->str); } /**
On Tue, Jun 25, 2024 at 11:56:18PM +0200, Thomas Weißschuh wrote: > On 2024-06-25 21:57:13+0000, Mark Brown wrote: > > <2>[ 50.236703] acpi LNXTHERM:08: Resources present before probing > This does make sense, the device is not yet bound to a driver. > Which apparently precludes the usage of devres. Oh, yes - I really wouldn't expect that to work at all, devres is all about tying things to the device being bound so trying to use it outside of that is not something I'd expect to go well. > I'm also wondering why the _STR attribute behaved differently in the > first place. > Does the patch below work better? That patch applied on top of -next appears to resolve the issue.
Rafael: Would you mind backing this series out for now? I'll do some more experiments and resubmit. On 2024-06-26 16:39:10+0000, Mark Brown wrote: > On Tue, Jun 25, 2024 at 11:56:18PM +0200, Thomas Weißschuh wrote: > > On 2024-06-25 21:57:13+0000, Mark Brown wrote: > > > > <2>[ 50.236703] acpi LNXTHERM:08: Resources present before probing > > > This does make sense, the device is not yet bound to a driver. > > Which apparently precludes the usage of devres. > > Oh, yes - I really wouldn't expect that to work at all, devres is all > about tying things to the device being bound so trying to use it outside > of that is not something I'd expect to go well. > > > I'm also wondering why the _STR attribute behaved differently in the > > first place. > > Does the patch below work better? > > That patch applied on top of -next appears to resolve the issue. Thanks for the confirmation
diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c index 4bedbe8f57ed..d0ca159d93e1 100644 --- a/drivers/acpi/device_sysfs.c +++ b/drivers/acpi/device_sysfs.c @@ -494,7 +494,7 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RO(status); -static const char *acpi_device_str(struct acpi_device *dev) +static const char *devm_acpi_device_str(struct acpi_device *dev) { struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL}; union acpi_object *str_obj; @@ -522,7 +522,7 @@ static const char *acpi_device_str(struct acpi_device *dev) buf, sizeof(buf) - 1); buf[result++] = '\0'; - ret = kstrdup(buf, GFP_KERNEL); + ret = devm_kstrdup(&dev->dev, buf, GFP_KERNEL); kfree(buffer.pointer); return ret; @@ -558,7 +558,7 @@ int acpi_device_setup_files(struct acpi_device *dev) /* * If device has _STR, 'description' file is created */ - dev->pnp.str = acpi_device_str(dev); + dev->pnp.str = devm_acpi_device_str(dev); if (dev->pnp.str) { result = device_create_file(&dev->dev, &dev_attr_description); if (result) @@ -632,10 +632,8 @@ void acpi_device_remove_files(struct acpi_device *dev) /* * If device has _STR, remove 'description' file */ - if (acpi_has_method(dev->handle, "_STR")) { - kfree(dev->pnp.str); + if (acpi_has_method(dev->handle, "_STR")) device_remove_file(&dev->dev, &dev_attr_description); - } /* * If device has _EJ0, remove 'eject' file. */
The string assigned to dev->pnp.str effectively shares the lifetime of the device. Use devm_-APIs to avoid a manual cleanup path. This will be useful when the attributes themselves will be managed by the device core. Signed-off-by: Thomas Weißschuh <linux@weissschuh.net> --- drivers/acpi/device_sysfs.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)