| 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 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); } /**
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(-)