| Message ID | 20240613-acpi-sysfs-groups-v1-1-665e0deb052a@weissschuh.net (mailing list archive) |
|---|---|
| State | Queued |
| Delegated to: | Rafael Wysocki |
| Headers | show |
| Series | ACPI: sysfs: manage sysfs attributes through device core | expand |
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 > >
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 > > > >
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 > > > > > >
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.
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)
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(-)