[PATCH 0/5] ACPI: sysfs: manage sysfs attributes through device core

[PATCH 0/5] ACPI: sysfs: manage sysfs attributes through device core

[Thomas Weißschuh]

Simplify the lifecycle of the sysfs attributes by letting the device
core manage them.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (5):
      ACPI: sysfs: convert utf-16 from _STR to utf-8 only once
      ACPI: sysfs: use device lifecycle for _STR result
      ACPI: sysfs: manage attributes as attribute_group
      ACPI: sysfs: manage sysfs attributes through device core
      ACPI: sysfs: remove return value of acpi_device_setup_files()

 drivers/acpi/device_sysfs.c | 229 +++++++++++++++++++++-----------------------
 drivers/acpi/internal.h     |   3 +-
 drivers/acpi/scan.c         |   6 +-
 include/acpi/acpi_bus.h     |   2 +-
 4 files changed, 114 insertions(+), 126 deletions(-)
---
base-commit: 7d91551085d3e7d5eb21c2481565b2be5288f11b
change-id: 20240609-acpi-sysfs-groups-cfa756d16752

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/5] ACPI: sysfs: convert utf-16 from _STR to utf-8 only once

[Thomas Weißschuh]

The ACPI _STR method returns an UTF-16 string that is converted to utf-8
before printing it in sysfs.
Currently this conversion is performed every time the "description"
sysfs attribute is read, which is not necessary.

Only perform the conversion once and cache the result.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/acpi/device_sysfs.c | 63 ++++++++++++++++++++++++++++-----------------
 include/acpi/acpi_bus.h     |  2 +-
 2 files changed, 40 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index 23373faa35ec..4bedbe8f57ed 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -439,24 +439,11 @@ static ssize_t description_show(struct device *dev,
 				char *buf)
 {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int result;
 
-	if (acpi_dev->pnp.str_obj == NULL)
+	if (acpi_dev->pnp.str == NULL)
 		return 0;
 
-	/*
-	 * 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 *)acpi_dev->pnp.str_obj->buffer.pointer,
-		acpi_dev->pnp.str_obj->buffer.length,
-		UTF16_LITTLE_ENDIAN, buf,
-		PAGE_SIZE - 1);
-
-	buf[result++] = '\n';
-
-	return result;
+	return sysfs_emit("%s\n", acpi_dev->pnp.str);
 }
 static DEVICE_ATTR_RO(description);
 
@@ -507,14 +494,46 @@ 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)
+{
+	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;
+}
+
 /**
  * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
  * @dev: ACPI device object.
  */
 int acpi_device_setup_files(struct acpi_device *dev)
 {
-	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
-	acpi_status status;
 	int result = 0;
 
 	/*
@@ -539,12 +558,8 @@ int acpi_device_setup_files(struct acpi_device *dev)
 	/*
 	 * If device has _STR, 'description' file is created
 	 */
-	if (acpi_has_method(dev->handle, "_STR")) {
-		status = acpi_evaluate_object(dev->handle, "_STR",
-					NULL, &buffer);
-		if (ACPI_FAILURE(status))
-			buffer.pointer = NULL;
-		dev->pnp.str_obj = buffer.pointer;
+	dev->pnp.str = acpi_device_str(dev);
+	if (dev->pnp.str) {
 		result = device_create_file(&dev->dev, &dev_attr_description);
 		if (result)
 			goto end;
@@ -618,7 +633,7 @@ 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_obj);
+		kfree(dev->pnp.str);
 		device_remove_file(&dev->dev, &dev_attr_description);
 	}
 	/*
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 1a4dfd7a1c4a..32e3105c9ece 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -254,7 +254,7 @@ struct acpi_device_pnp {
 	struct list_head ids;		/* _HID and _CIDs */
 	acpi_device_name device_name;	/* Driver-determined */
 	acpi_device_class device_class;	/*        "          */
-	union acpi_object *str_obj;	/* unicode string for _STR method */
+	const char *str;		/* _STR */
 };
 
 #define acpi_device_bid(d)	((d)->pnp.bus_id)

-- 
2.45.2

Re: [PATCH 1/5] ACPI: sysfs: convert utf-16 from _STR to utf-8 only once

[Rafael J. Wysocki]

On Thu, Jun 13, 2024 at 10:15 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> The ACPI _STR method returns an UTF-16 string that is converted to utf-8
> before printing it in sysfs.
> Currently this conversion is performed every time the "description"
> sysfs attribute is read, which is not necessary.

Why is it a problem, though?

How many devices have _STR and how much of the time is it used?

Hint: On the system I'm sitting in front of, the answer is 0 and never.

So Is it really worth adding an _STR string pointer to every struct acpi_device?

> Only perform the conversion once and cache the result.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/acpi/device_sysfs.c | 63 ++++++++++++++++++++++++++++-----------------
>  include/acpi/acpi_bus.h     |  2 +-
>  2 files changed, 40 insertions(+), 25 deletions(-)

And it's more lines of code even.

>
> diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> index 23373faa35ec..4bedbe8f57ed 100644
> --- a/drivers/acpi/device_sysfs.c
> +++ b/drivers/acpi/device_sysfs.c
> @@ -439,24 +439,11 @@ static ssize_t description_show(struct device *dev,
>                                 char *buf)
>  {
>         struct acpi_device *acpi_dev = to_acpi_device(dev);
> -       int result;
>
> -       if (acpi_dev->pnp.str_obj == NULL)
> +       if (acpi_dev->pnp.str == NULL)
>                 return 0;
>
> -       /*
> -        * 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 *)acpi_dev->pnp.str_obj->buffer.pointer,
> -               acpi_dev->pnp.str_obj->buffer.length,
> -               UTF16_LITTLE_ENDIAN, buf,
> -               PAGE_SIZE - 1);
> -
> -       buf[result++] = '\n';
> -
> -       return result;
> +       return sysfs_emit("%s\n", acpi_dev->pnp.str);
>  }
>  static DEVICE_ATTR_RO(description);
>
> @@ -507,14 +494,46 @@ 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)
> +{
> +       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;
> +}
> +
>  /**
>   * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
>   * @dev: ACPI device object.
>   */
>  int acpi_device_setup_files(struct acpi_device *dev)
>  {
> -       struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> -       acpi_status status;
>         int result = 0;
>
>         /*
> @@ -539,12 +558,8 @@ int acpi_device_setup_files(struct acpi_device *dev)
>         /*
>          * If device has _STR, 'description' file is created
>          */
> -       if (acpi_has_method(dev->handle, "_STR")) {
> -               status = acpi_evaluate_object(dev->handle, "_STR",
> -                                       NULL, &buffer);
> -               if (ACPI_FAILURE(status))
> -                       buffer.pointer = NULL;
> -               dev->pnp.str_obj = buffer.pointer;
> +       dev->pnp.str = acpi_device_str(dev);
> +       if (dev->pnp.str) {
>                 result = device_create_file(&dev->dev, &dev_attr_description);
>                 if (result)
>                         goto end;
> @@ -618,7 +633,7 @@ 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_obj);
> +               kfree(dev->pnp.str);
>                 device_remove_file(&dev->dev, &dev_attr_description);
>         }
>         /*
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 1a4dfd7a1c4a..32e3105c9ece 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -254,7 +254,7 @@ struct acpi_device_pnp {
>         struct list_head ids;           /* _HID and _CIDs */
>         acpi_device_name device_name;   /* Driver-determined */
>         acpi_device_class device_class; /*        "          */
> -       union acpi_object *str_obj;     /* unicode string for _STR method */
> +       const char *str;                /* _STR */
>  };
>
>  #define acpi_device_bid(d)     ((d)->pnp.bus_id)
>
> --
> 2.45.2
>
>

Re: [PATCH 1/5] ACPI: sysfs: convert utf-16 from _STR to utf-8 only once

[Rafael J. Wysocki]

On Mon, Jun 17, 2024 at 8:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Jun 13, 2024 at 10:15 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > The ACPI _STR method returns an UTF-16 string that is converted to utf-8
> > before printing it in sysfs.
> > Currently this conversion is performed every time the "description"
> > sysfs attribute is read, which is not necessary.
>
> Why is it a problem, though?
>
> How many devices have _STR and how much of the time is it used?
>
> Hint: On the system I'm sitting in front of, the answer is 0 and never.

This was actually factually incorrect, sorry.

The correct answer is 12 out of 247 and very rarely (if at all).
Which doesn't really change the point IMO.

> So Is it really worth adding an _STR string pointer to every struct acpi_device?
>
> > Only perform the conversion once and cache the result.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  drivers/acpi/device_sysfs.c | 63 ++++++++++++++++++++++++++++-----------------
> >  include/acpi/acpi_bus.h     |  2 +-
> >  2 files changed, 40 insertions(+), 25 deletions(-)
>
> And it's more lines of code even.
>
> >
> > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > index 23373faa35ec..4bedbe8f57ed 100644
> > --- a/drivers/acpi/device_sysfs.c
> > +++ b/drivers/acpi/device_sysfs.c
> > @@ -439,24 +439,11 @@ static ssize_t description_show(struct device *dev,
> >                                 char *buf)
> >  {
> >         struct acpi_device *acpi_dev = to_acpi_device(dev);
> > -       int result;
> >
> > -       if (acpi_dev->pnp.str_obj == NULL)
> > +       if (acpi_dev->pnp.str == NULL)
> >                 return 0;
> >
> > -       /*
> > -        * 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 *)acpi_dev->pnp.str_obj->buffer.pointer,
> > -               acpi_dev->pnp.str_obj->buffer.length,
> > -               UTF16_LITTLE_ENDIAN, buf,
> > -               PAGE_SIZE - 1);
> > -
> > -       buf[result++] = '\n';
> > -
> > -       return result;
> > +       return sysfs_emit("%s\n", acpi_dev->pnp.str);
> >  }
> >  static DEVICE_ATTR_RO(description);
> >
> > @@ -507,14 +494,46 @@ 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)
> > +{
> > +       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;
> > +}
> > +
> >  /**
> >   * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
> >   * @dev: ACPI device object.
> >   */
> >  int acpi_device_setup_files(struct acpi_device *dev)
> >  {
> > -       struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > -       acpi_status status;
> >         int result = 0;
> >
> >         /*
> > @@ -539,12 +558,8 @@ int acpi_device_setup_files(struct acpi_device *dev)
> >         /*
> >          * If device has _STR, 'description' file is created
> >          */
> > -       if (acpi_has_method(dev->handle, "_STR")) {
> > -               status = acpi_evaluate_object(dev->handle, "_STR",
> > -                                       NULL, &buffer);
> > -               if (ACPI_FAILURE(status))
> > -                       buffer.pointer = NULL;
> > -               dev->pnp.str_obj = buffer.pointer;
> > +       dev->pnp.str = acpi_device_str(dev);
> > +       if (dev->pnp.str) {
> >                 result = device_create_file(&dev->dev, &dev_attr_description);
> >                 if (result)
> >                         goto end;
> > @@ -618,7 +633,7 @@ 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_obj);
> > +               kfree(dev->pnp.str);
> >                 device_remove_file(&dev->dev, &dev_attr_description);
> >         }
> >         /*
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index 1a4dfd7a1c4a..32e3105c9ece 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -254,7 +254,7 @@ struct acpi_device_pnp {
> >         struct list_head ids;           /* _HID and _CIDs */
> >         acpi_device_name device_name;   /* Driver-determined */
> >         acpi_device_class device_class; /*        "          */
> > -       union acpi_object *str_obj;     /* unicode string for _STR method */
> > +       const char *str;                /* _STR */
> >  };
> >
> >  #define acpi_device_bid(d)     ((d)->pnp.bus_id)
> >
> > --
> > 2.45.2
> >
> >

Re: [PATCH 1/5] ACPI: sysfs: convert utf-16 from _STR to utf-8 only once

[Thomas Weißschuh]

On 2024-06-17 20:43:03+0000, Rafael J. Wysocki wrote:
> On Mon, Jun 17, 2024 at 8:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Thu, Jun 13, 2024 at 10:15 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > >
> > > The ACPI _STR method returns an UTF-16 string that is converted to utf-8
> > > before printing it in sysfs.
> > > Currently this conversion is performed every time the "description"
> > > sysfs attribute is read, which is not necessary.
> >
> > Why is it a problem, though?

It's not a real problem, mostly it made the following changes simpler.

> > How many devices have _STR and how much of the time is it used?
> >
> > Hint: On the system I'm sitting in front of, the answer is 0 and never.
> 
> This was actually factually incorrect, sorry.
> 
> The correct answer is 12 out of 247 and very rarely (if at all).
> Which doesn't really change the point IMO.
> 
> > So Is it really worth adding an _STR string pointer to every struct acpi_device?

The string pointer replaces a 'union acpi_object *str_obj', so no new
space is used.
Also in case the device _STR is present the new code uses less memory, as
it doesn't need the full union and stores utf-8 instead of utf-16.
(Plus a few more cycles for the preemptive conversion)

In case no _STR is present both CPU and memory costs are identical.

Anyways, I don't really care about this and can also try to drop this
patch if you prefer.
Or drop the 'union acpi_device *' from the struct completely at your
preference.

> > > Only perform the conversion once and cache the result.
> > >
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > >  drivers/acpi/device_sysfs.c | 63 ++++++++++++++++++++++++++++-----------------
> > >  include/acpi/acpi_bus.h     |  2 +-
> > >  2 files changed, 40 insertions(+), 25 deletions(-)
> >
> > And it's more lines of code even.
> >
> > >
> > > diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
> > > index 23373faa35ec..4bedbe8f57ed 100644
> > > --- a/drivers/acpi/device_sysfs.c
> > > +++ b/drivers/acpi/device_sysfs.c
> > > @@ -439,24 +439,11 @@ static ssize_t description_show(struct device *dev,
> > >                                 char *buf)
> > >  {
> > >         struct acpi_device *acpi_dev = to_acpi_device(dev);
> > > -       int result;
> > >
> > > -       if (acpi_dev->pnp.str_obj == NULL)
> > > +       if (acpi_dev->pnp.str == NULL)
> > >                 return 0;
> > >
> > > -       /*
> > > -        * 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 *)acpi_dev->pnp.str_obj->buffer.pointer,
> > > -               acpi_dev->pnp.str_obj->buffer.length,
> > > -               UTF16_LITTLE_ENDIAN, buf,
> > > -               PAGE_SIZE - 1);
> > > -
> > > -       buf[result++] = '\n';
> > > -
> > > -       return result;
> > > +       return sysfs_emit("%s\n", acpi_dev->pnp.str);
> > >  }
> > >  static DEVICE_ATTR_RO(description);
> > >
> > > @@ -507,14 +494,46 @@ 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)
> > > +{
> > > +       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;
> > > +}
> > > +
> > >  /**
> > >   * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
> > >   * @dev: ACPI device object.
> > >   */
> > >  int acpi_device_setup_files(struct acpi_device *dev)
> > >  {
> > > -       struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
> > > -       acpi_status status;
> > >         int result = 0;
> > >
> > >         /*
> > > @@ -539,12 +558,8 @@ int acpi_device_setup_files(struct acpi_device *dev)
> > >         /*
> > >          * If device has _STR, 'description' file is created
> > >          */
> > > -       if (acpi_has_method(dev->handle, "_STR")) {
> > > -               status = acpi_evaluate_object(dev->handle, "_STR",
> > > -                                       NULL, &buffer);
> > > -               if (ACPI_FAILURE(status))
> > > -                       buffer.pointer = NULL;
> > > -               dev->pnp.str_obj = buffer.pointer;
> > > +       dev->pnp.str = acpi_device_str(dev);
> > > +       if (dev->pnp.str) {
> > >                 result = device_create_file(&dev->dev, &dev_attr_description);
> > >                 if (result)
> > >                         goto end;
> > > @@ -618,7 +633,7 @@ 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_obj);
> > > +               kfree(dev->pnp.str);
> > >                 device_remove_file(&dev->dev, &dev_attr_description);
> > >         }
> > >         /*
> > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > > index 1a4dfd7a1c4a..32e3105c9ece 100644
> > > --- a/include/acpi/acpi_bus.h
> > > +++ b/include/acpi/acpi_bus.h
> > > @@ -254,7 +254,7 @@ struct acpi_device_pnp {
> > >         struct list_head ids;           /* _HID and _CIDs */
> > >         acpi_device_name device_name;   /* Driver-determined */
> > >         acpi_device_class device_class; /*        "          */
> > > -       union acpi_object *str_obj;     /* unicode string for _STR method */
> > > +       const char *str;                /* _STR */
> > >  };
> > >
> > >  #define acpi_device_bid(d)     ((d)->pnp.bus_id)
> > >
> > > --
> > > 2.45.2
> > >
> > >

Re: [PATCH 1/5] ACPI: sysfs: convert utf-16 from _STR to utf-8 only once

[Rafael J. Wysocki]

On Mon, Jun 17, 2024 at 8:57 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2024-06-17 20:43:03+0000, Rafael J. Wysocki wrote:
> > On Mon, Jun 17, 2024 at 8:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Thu, Jun 13, 2024 at 10:15 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > > >
> > > > The ACPI _STR method returns an UTF-16 string that is converted to utf-8
> > > > before printing it in sysfs.
> > > > Currently this conversion is performed every time the "description"
> > > > sysfs attribute is read, which is not necessary.
> > >
> > > Why is it a problem, though?
>
> It's not a real problem, mostly it made the following changes simpler.
>
> > > How many devices have _STR and how much of the time is it used?
> > >
> > > Hint: On the system I'm sitting in front of, the answer is 0 and never.
> >
> > This was actually factually incorrect, sorry.
> >
> > The correct answer is 12 out of 247 and very rarely (if at all).
> > Which doesn't really change the point IMO.
> >
> > > So Is it really worth adding an _STR string pointer to every struct acpi_device?
>
> The string pointer replaces a 'union acpi_object *str_obj', so no new
> space is used.
> Also in case the device _STR is present the new code uses less memory, as
> it doesn't need the full union and stores utf-8 instead of utf-16.
> (Plus a few more cycles for the preemptive conversion)
>
> In case no _STR is present both CPU and memory costs are identical.

OK

> Anyways, I don't really care about this and can also try to drop this
> patch if you prefer.
> Or drop the 'union acpi_device *' from the struct completely at your
> preference.

No, this is fine as is, thanks.

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

[Thomas Weißschuh]

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(-)

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.
 	 */

-- 
2.45.2

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4999 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Thomas Weißschuh]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Thomas Weißschuh]

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

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

[Rafael J. Wysocki]

On Thu, Jun 27, 2024 at 7:17 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Rafael:
>
> Would you mind backing this series out for now?
> I'll do some more experiments and resubmit.

Done, thank you!

[PATCH 3/5] ACPI: sysfs: manage attributes as attribute_group

[Thomas Weißschuh]

The current manual attribute management is inconsistent and brittle.
Not all return values of device_create_file() are checked and the
cleanup logic needs to be kept up to date manually.

Moving all attributes into an attribute_group and using the is_visible()
callback allows the management of all attributes as a single unit.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/acpi/device_sysfs.c | 190 ++++++++++++++++++++------------------------
 1 file changed, 84 insertions(+), 106 deletions(-)

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index d0ca159d93e1..a673488066b3 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -494,6 +494,88 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(status);
 
+static struct attribute *acpi_attrs[] = {
+	&dev_attr_path.attr,
+	&dev_attr_hid.attr,
+	&dev_attr_modalias.attr,
+	&dev_attr_description.attr,
+	&dev_attr_adr.attr,
+	&dev_attr_uid.attr,
+	&dev_attr_sun.attr,
+	&dev_attr_hrv.attr,
+	&dev_attr_status.attr,
+	&dev_attr_eject.attr,
+	&dev_attr_power_state.attr,
+	&dev_attr_real_power_state.attr,
+	NULL
+};
+
+static bool acpi_show_attr(struct acpi_device *dev, const struct device_attribute *attr)
+{
+	/*
+	 * Devices gotten from FADT don't have a "path" attribute
+	 */
+	if (attr == &dev_attr_path)
+		return dev->handle;
+
+	if (attr == &dev_attr_hid || attr == &dev_attr_modalias)
+		return !list_empty(&dev->pnp.ids);
+
+	/*
+	 * If device has _STR, 'description' file is created
+	 */
+	if (attr == &dev_attr_description)
+		return dev->pnp.str;
+
+	if (attr == &dev_attr_adr)
+		return dev->pnp.type.bus_address;
+
+	if (attr == &dev_attr_uid)
+		return acpi_device_uid(dev);
+
+	if (attr == &dev_attr_sun)
+		return acpi_has_method(dev->handle, "_SUN");
+
+	if (attr == &dev_attr_hrv)
+		return acpi_has_method(dev->handle, "_HRV");
+
+	if (attr == &dev_attr_status)
+		return acpi_has_method(dev->handle, "_STA");
+
+	/*
+	 * If device has _EJ0, 'eject' file is created that is used to trigger
+	 * hot-removal function from userland.
+	 */
+	if (attr == &dev_attr_eject)
+		return acpi_has_method(dev->handle, "_EJ0");
+
+	if (attr == &dev_attr_power_state)
+		return dev->flags.power_manageable;
+
+	if (attr == &dev_attr_real_power_state)
+		return dev->flags.power_manageable && dev->power.flags.power_resources;
+
+	dev_warn_once(&dev->dev, "Unexpected attribute: %s\n", attr->attr.name);
+	return false;
+}
+
+static umode_t acpi_attr_is_visible(struct kobject *kobj,
+				    struct attribute *attr,
+				    int attrno)
+{
+	struct acpi_device *dev = to_acpi_device(kobj_to_dev(kobj));
+
+	if (acpi_show_attr(dev, container_of(attr, struct device_attribute, attr)))
+		return attr->mode;
+	else
+		return 0;
+}
+
+static const struct attribute_group acpi_group = {
+	.attrs = acpi_attrs,
+	.is_visible = acpi_attr_is_visible,
+};
+
 static const char *devm_acpi_device_str(struct acpi_device *dev)
 {
 	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
@@ -536,81 +618,11 @@ int acpi_device_setup_files(struct acpi_device *dev)
 {
 	int result = 0;
 
-	/*
-	 * Devices gotten from FADT don't have a "path" attribute
-	 */
-	if (dev->handle) {
-		result = device_create_file(&dev->dev, &dev_attr_path);
-		if (result)
-			goto end;
-	}
-
-	if (!list_empty(&dev->pnp.ids)) {
-		result = device_create_file(&dev->dev, &dev_attr_hid);
-		if (result)
-			goto end;
-
-		result = device_create_file(&dev->dev, &dev_attr_modalias);
-		if (result)
-			goto end;
-	}
-
-	/*
-	 * If device has _STR, 'description' file is created
-	 */
 	dev->pnp.str = devm_acpi_device_str(dev);
-	if (dev->pnp.str) {
-		result = device_create_file(&dev->dev, &dev_attr_description);
-		if (result)
-			goto end;
-	}
-
-	if (dev->pnp.type.bus_address)
-		result = device_create_file(&dev->dev, &dev_attr_adr);
-	if (acpi_device_uid(dev))
-		result = device_create_file(&dev->dev, &dev_attr_uid);
-
-	if (acpi_has_method(dev->handle, "_SUN")) {
-		result = device_create_file(&dev->dev, &dev_attr_sun);
-		if (result)
-			goto end;
-	}
-
-	if (acpi_has_method(dev->handle, "_HRV")) {
-		result = device_create_file(&dev->dev, &dev_attr_hrv);
-		if (result)
-			goto end;
-	}
-
-	if (acpi_has_method(dev->handle, "_STA")) {
-		result = device_create_file(&dev->dev, &dev_attr_status);
-		if (result)
-			goto end;
-	}
-
-	/*
-	 * If device has _EJ0, 'eject' file is created that is used to trigger
-	 * hot-removal function from userland.
-	 */
-	if (acpi_has_method(dev->handle, "_EJ0")) {
-		result = device_create_file(&dev->dev, &dev_attr_eject);
-		if (result)
-			return result;
-	}
-
-	if (dev->flags.power_manageable) {
-		result = device_create_file(&dev->dev, &dev_attr_power_state);
-		if (result)
-			return result;
-
-		if (dev->power.flags.power_resources)
-			result = device_create_file(&dev->dev,
-						    &dev_attr_real_power_state);
-	}
+	result = device_add_group(&dev->dev, &acpi_group);
 
 	acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
 
-end:
 	return result;
 }
 
@@ -621,39 +633,5 @@ int acpi_device_setup_files(struct acpi_device *dev)
 void acpi_device_remove_files(struct acpi_device *dev)
 {
 	acpi_hide_nondev_subnodes(&dev->data);
-
-	if (dev->flags.power_manageable) {
-		device_remove_file(&dev->dev, &dev_attr_power_state);
-		if (dev->power.flags.power_resources)
-			device_remove_file(&dev->dev,
-					   &dev_attr_real_power_state);
-	}
-
-	/*
-	 * If device has _STR, remove 'description' file
-	 */
-	if (acpi_has_method(dev->handle, "_STR"))
-		device_remove_file(&dev->dev, &dev_attr_description);
-	/*
-	 * If device has _EJ0, remove 'eject' file.
-	 */
-	if (acpi_has_method(dev->handle, "_EJ0"))
-		device_remove_file(&dev->dev, &dev_attr_eject);
-
-	if (acpi_has_method(dev->handle, "_SUN"))
-		device_remove_file(&dev->dev, &dev_attr_sun);
-
-	if (acpi_has_method(dev->handle, "_HRV"))
-		device_remove_file(&dev->dev, &dev_attr_hrv);
-
-	if (acpi_device_uid(dev))
-		device_remove_file(&dev->dev, &dev_attr_uid);
-	if (dev->pnp.type.bus_address)
-		device_remove_file(&dev->dev, &dev_attr_adr);
-	device_remove_file(&dev->dev, &dev_attr_modalias);
-	device_remove_file(&dev->dev, &dev_attr_hid);
-	if (acpi_has_method(dev->handle, "_STA"))
-		device_remove_file(&dev->dev, &dev_attr_status);
-	if (dev->handle)
-		device_remove_file(&dev->dev, &dev_attr_path);
+	device_remove_group(&dev->dev, &acpi_group);
 }

-- 
2.45.2

[PATCH 4/5] ACPI: sysfs: manage sysfs attributes through device core

[Thomas Weißschuh]

Now that the acpi sysfs attributes are organized around an
attribute_group the device core can manage them.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/acpi/device_sysfs.c | 8 +++++---
 drivers/acpi/internal.h     | 1 +
 drivers/acpi/scan.c         | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index a673488066b3..f1e8928254c2 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -576,6 +576,11 @@ static const struct attribute_group acpi_group = {
 	.is_visible = acpi_attr_is_visible,
 };
 
+const struct attribute_group *acpi_groups[] = {
+	&acpi_group,
+	NULL
+};
+
 static const char *devm_acpi_device_str(struct acpi_device *dev)
 {
 	struct acpi_buffer buffer = {ACPI_ALLOCATE_BUFFER, NULL};
@@ -619,8 +624,6 @@ int acpi_device_setup_files(struct acpi_device *dev)
 	int result = 0;
 
 	dev->pnp.str = devm_acpi_device_str(dev);
-	result = device_add_group(&dev->dev, &acpi_group);
-
 	acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
 
 	return result;
@@ -633,5 +636,4 @@ int acpi_device_setup_files(struct acpi_device *dev)
 void acpi_device_remove_files(struct acpi_device *dev)
 {
 	acpi_hide_nondev_subnodes(&dev->data);
-	device_remove_group(&dev->dev, &acpi_group);
 }
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 2a0e9fc7b74c..63dd78d80508 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -120,6 +120,7 @@ int acpi_tie_acpi_dev(struct acpi_device *adev);
 int acpi_device_add(struct acpi_device *device);
 int acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
+extern const struct attribute_group *acpi_groups[];
 void acpi_device_add_finalize(struct acpi_device *device);
 void acpi_free_pnp_ids(struct acpi_device_pnp *pnp);
 bool acpi_device_is_enabled(const struct acpi_device *adev);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 503773707e01..c15fffefca0a 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1813,6 +1813,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	device->dev.parent = parent ? &parent->dev : NULL;
 	device->dev.release = release;
 	device->dev.bus = &acpi_bus_type;
+	device->dev.groups = acpi_groups;
 	fwnode_init(&device->fwnode, &acpi_device_fwnode_ops);
 	acpi_set_device_status(device, ACPI_STA_DEFAULT);
 	acpi_device_get_busid(device);

-- 
2.45.2

[PATCH 5/5] ACPI: sysfs: remove return value of acpi_device_setup_files()

[Thomas Weißschuh]

The function can not fail anymore, so drop its return value.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/acpi/device_sysfs.c | 6 +-----
 drivers/acpi/internal.h     | 2 +-
 drivers/acpi/scan.c         | 5 +----
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/device_sysfs.c b/drivers/acpi/device_sysfs.c
index f1e8928254c2..c85ec754931c 100644
--- a/drivers/acpi/device_sysfs.c
+++ b/drivers/acpi/device_sysfs.c
@@ -619,14 +619,10 @@ static const char *devm_acpi_device_str(struct acpi_device *dev)
  * acpi_device_setup_files - Create sysfs attributes of an ACPI device.
  * @dev: ACPI device object.
  */
-int acpi_device_setup_files(struct acpi_device *dev)
+void acpi_device_setup_files(struct acpi_device *dev)
 {
-	int result = 0;
-
 	dev->pnp.str = devm_acpi_device_str(dev);
 	acpi_expose_nondev_subnodes(&dev->dev.kobj, &dev->data);
-
-	return result;
 }
 
 /**
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 63dd78d80508..e71fecbf731c 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -118,7 +118,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 			     int type, void (*release)(struct device *));
 int acpi_tie_acpi_dev(struct acpi_device *adev);
 int acpi_device_add(struct acpi_device *device);
-int acpi_device_setup_files(struct acpi_device *dev);
+void acpi_device_setup_files(struct acpi_device *dev);
 void acpi_device_remove_files(struct acpi_device *dev);
 extern const struct attribute_group *acpi_groups[];
 void acpi_device_add_finalize(struct acpi_device *device);
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c15fffefca0a..49a8172fe0de 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -766,10 +766,7 @@ int acpi_device_add(struct acpi_device *device)
 		goto err;
 	}
 
-	result = acpi_device_setup_files(device);
-	if (result)
-		pr_err("Error creating sysfs interface for device %s\n",
-		       dev_name(&device->dev));
+	acpi_device_setup_files(device);
 
 	return 0;
 

-- 
2.45.2

Re: [PATCH 0/5] ACPI: sysfs: manage sysfs attributes through device core

[Rafael J. Wysocki]

On Thu, Jun 13, 2024 at 10:15 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Simplify the lifecycle of the sysfs attributes by letting the device
> core manage them.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Thomas Weißschuh (5):
>       ACPI: sysfs: convert utf-16 from _STR to utf-8 only once
>       ACPI: sysfs: use device lifecycle for _STR result
>       ACPI: sysfs: manage attributes as attribute_group
>       ACPI: sysfs: manage sysfs attributes through device core
>       ACPI: sysfs: remove return value of acpi_device_setup_files()
>
>  drivers/acpi/device_sysfs.c | 229 +++++++++++++++++++++-----------------------
>  drivers/acpi/internal.h     |   3 +-
>  drivers/acpi/scan.c         |   6 +-
>  include/acpi/acpi_bus.h     |   2 +-
>  4 files changed, 114 insertions(+), 126 deletions(-)
> ---

Whole series applied as 6.11 material, thanks!