Re: [PATCH 2/5] ACPI: sysfs: use device lifecycle for _STR result

["Thomas Weißschuh" <linux@weissschuh.net>]

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