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