Deduplicate bin_attribute simple read() callbacks

When drivers expose a bin_attribute in sysfs which is backed by a buffer in memory, a common pattern is to set the @private and @size members in struct bin_attribute to the buffer's location and size.

The ->read() callback then merely consists of a single memcpy() call. It's not even necessary to perform bounds checks as these are already handled by sysfs_kf_bin_read().

However each driver is so far providing its own ->read() implementation. The pattern is sufficiently frequent to merit a public helper, so add sysfs_bin_attr_simple_read() as well as BIN_ATTR_SIMPLE_RO() and BIN_ATTR_SIMPLE_ADMIN_RO() macros to ease declaration of such bin_attributes and reduce LoC and .text section size.

Signed-off-by Lukas Wunner <[email protected]>

---
 fs/sysfs/file.c       | 27 +++++++++++++++++++++++++++
 include/linux/sysfs.h | 15 +++++++++++++++
 2 files changed, 42 insertions(+)

Deduplicate ->read() callbacks of bin_attributes which are backed by a simple buffer in memory:

Use the newly introduced sysfs_bin_attr_simple_read() helper instead, either by referencing it directly or by declaring such bin_attributes with BIN_ATTR_SIMPLE_RO() or BIN_ATTR_SIMPLE_ADMIN_RO().

Aside from a reduction of LoC, this shaves off a few bytes from vmlinux (304 bytes on an x86_64 allyesconfig).

No functional change intended.

Signed-off-by Lukas Wunner <[email protected]>

---
 arch/powerpc/platforms/powernv/opal.c              | 10 +--------
 drivers/acpi/bgrt.c                                |  9 +-------
 drivers/firmware/dmi_scan.c                        | 12 ++--------
 drivers/firmware/efi/rci2-table.c                  | 10 +--------
 drivers/gpu/drm/i915/gvt/firmware.c                | 26
+++++----------------- .../intel/int340x_thermal/int3400_thermal.c
    |  9 +------- init/initramfs.c

| 10 +-------- kernel/module/sysfs.c |
13 +---------- 8 files changed, 14 insertions(+), 85 deletions(-)

(quoted diff) +0 -0

b/arch/powerpc/platforms/powernv/opal.c index 45dd77e..5d0f35b 100644

arch/powerpc/platforms/powernv/opal.c +0 -8

@@ -792,14 +792,6 @@ static int __init opal_sysfs_init(void)
 	return 0;
 }
 
-static ssize_t export_attr_read(struct file *fp, struct kobject
 *kobj,
-				struct bin_attribute *bin_attr, char
 *buf,
-				loff_t off, size_t count)
-{
-	return memory_read_from_buffer(buf, count, &off,
 bin_attr->private,
-				       bin_attr->size);
-}
-

static int opal_add_one_export(struct kobject *parent, const char
*export_name, struct device_node *np, const char *prop_name)
{

arch/powerpc/platforms/powernv/opal.c +1 -1

@@ -826,7 +818,7 @@ static int opal_add_one_export(struct kobject
 *parent, const char *export_name, sysfs_bin_attr_init(attr);
 	attr->attr.name = name;
 	attr->attr.mode = 0400;
-	attr->read = export_attr_read;
+	attr->read = sysfs_bin_attr_simple_read;
 	attr->private = __va(vals[0]);
 	attr->size = vals[1];
 

drivers/acpi/bgrt.c +1 -7

index e4fb9e2..d1d9c92 100644
@@ -29,14 +29,7 @@
 BGRT_SHOW(xoffset, image_offset_x);
 BGRT_SHOW(yoffset, image_offset_y);
 
-static ssize_t image_read(struct file *file, struct kobject *kobj,
-	       struct bin_attribute *attr, char *buf, loff_t off,
 size_t count) -{
-	memcpy(buf, attr->private + off, count);
-	return count;
-}
-
-static BIN_ATTR_RO(image, 0);	/* size gets filled in later */
+static BIN_ATTR_SIMPLE_RO(image);
 
 static struct attribute *bgrt_attributes[] = {
 	&bgrt_attr_version.attr,

drivers/firmware/dmi_scan.c +2 -9

index 015c95a..3d0f773 100644
@@ -746,16 +746,8 @@ static void __init dmi_scan_machine(void)
 	pr_info("DMI not present or invalid.\n");
 }
 
-static ssize_t raw_table_read(struct file *file, struct kobject
 *kobj,
-			      struct bin_attribute *attr, char *buf,
-			      loff_t pos, size_t count)
-{
-	memcpy(buf, attr->private + pos, count);
-	return count;
-}
-
-static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL,
 0); -static BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
+static BIN_ATTR_SIMPLE_ADMIN_RO(smbios_entry_point);
+static BIN_ATTR_SIMPLE_ADMIN_RO(DMI);
 
 static int __init dmi_init(void)
 {

(quoted diff) +0 -0

b/drivers/firmware/efi/rci2-table.c index de1a9a1..4fd45d6 100644

drivers/firmware/efi/rci2-table.c +1 -9

@@ -40,15 +40,7 @@ struct rci2_table_global_hdr {
 static u32 rci2_table_len;
 unsigned long rci2_table_phys __ro_after_init =
 EFI_INVALID_TABLE_ADDR; 
-static ssize_t raw_table_read(struct file *file, struct kobject
 *kobj,
-			      struct bin_attribute *attr, char *buf,
-			      loff_t pos, size_t count)
-{
-	memcpy(buf, attr->private + pos, count);
-	return count;
-}
-
-static BIN_ATTR(rci2, S_IRUSR, raw_table_read, NULL, 0);
+static BIN_ATTR_SIMPLE_ADMIN_RO(rci2);
 
 static u16 checksum(void)

{

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/gvt/firmware.c index 4dd52ac..5e66a26 100644

drivers/gpu/drm/i915/gvt/firmware.c +1 -15

@@ -50,21 +50,7 @@ struct gvt_firmware_header {
 
 #define dev_to_drm_minor(d) dev_get_drvdata((d))
 
-static ssize_t
-gvt_firmware_read(struct file *filp, struct kobject *kobj,
-	     struct bin_attribute *attr, char *buf,
-	     loff_t offset, size_t count)
-{
-	memcpy(buf, attr->private + offset, count);
-	return count;
-}
-
-static struct bin_attribute firmware_attr = {
-	.attr = {.name = "gvt_firmware", .mode = (S_IRUSR)},
-	.read = gvt_firmware_read,
-	.write = NULL,
-	.mmap = NULL,
-};
+static BIN_ATTR_SIMPLE_ADMIN_RO(gvt_firmware);
 
 static int expose_firmware_sysfs(struct intel_gvt *gvt)
 {

@@ -107,10 +93,10 @@ static int expose_firmware_sysfs(struct
 intel_gvt *gvt) crc32_start = offsetof(struct gvt_firmware_header,

version); h->crc32 = crc32_le(0, firmware + crc32_start, size - crc32_start);

drivers/gpu/drm/i915/gvt/firmware.c +3 -3

-	firmware_attr.size = size;
-	firmware_attr.private = firmware;
+	bin_attr_gvt_firmware.size = size;
+	bin_attr_gvt_firmware.private = firmware;
 
-	ret = device_create_bin_file(&pdev->dev, &firmware_attr);
+	ret = device_create_bin_file(&pdev->dev,

&bin_attr_gvt_firmware); if (ret) {

drivers/gpu/drm/i915/gvt/firmware.c +2 -2

 		vfree(firmware);
 		return ret;

@@ -122,8 +108,8 @@ static void clean_firmware_sysfs(struct intel_gvt
 *gvt) {
 	struct pci_dev *pdev = to_pci_dev(gvt->gt->i915->drm.dev);
 
-	device_remove_bin_file(&pdev->dev, &firmware_attr);
-	vfree(firmware_attr.private);
+	device_remove_bin_file(&pdev->dev, &bin_attr_gvt_firmware);
+	vfree(bin_attr_gvt_firmware.private);
 }
 
 /**

(quoted diff) +0 -0

b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c index 427d370..6d4b51a 100644 --- a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c +++ b/drivers/thermal/intel/int340x_thermal/int3400_thermal.c @@ -73,14 +73,7 @@ struct odvp_attr { struct device_attribute attr;
};

-static ssize_t data_vault_read(struct file *file, struct kobject *kobj,

(quoted diff) +0 -1

-	     struct bin_attribute *attr, char *buf, loff_t off,

size_t count) -{

drivers/thermal/intel/int340x_thermal/int3400_thermal.c +1 -5

-	memcpy(buf, attr->private + off, count);
-	return count;
-}
-
-static BIN_ATTR_RO(data_vault, 0);
+static BIN_ATTR_SIMPLE_RO(data_vault);
 
 static struct bin_attribute *data_attributes[] = {
 	&bin_attr_data_vault,

init/initramfs.c +1 -9

index da79760..5193fae 100644
@@ -575,15 +575,7 @@ static int __init initramfs_async_setup(char
 *str) #include <linux/initrd.h>
 #include <linux/kexec.h>
 
-static ssize_t raw_read(struct file *file, struct kobject *kobj,
-			struct bin_attribute *attr, char *buf,
-			loff_t pos, size_t count)
-{
-	memcpy(buf, attr->private + pos, count);
-	return count;
-}
-
-static BIN_ATTR(initrd, 0440, raw_read, NULL, 0);
+static BIN_ATTR(initrd, 0440, sysfs_bin_attr_simple_read, NULL, 0);
 
 void __init reserve_initrd_mem(void)
 {

kernel/module/sysfs.c +0 -11

index d964167..26efe13 100644
@@ -146,17 +146,6 @@ struct module_notes_attrs {
 	struct bin_attribute attrs[] __counted_by(notes);
 };
 
-static ssize_t module_notes_read(struct file *filp, struct kobject
 *kobj,
-				 struct bin_attribute *bin_attr,
-				 char *buf, loff_t pos, size_t count)
-{
-	/*
-	 * The caller checked the pos and count against our size.
-	 */
-	memcpy(buf, bin_attr->private + pos, count);
-	return count;
-}
-
 static void free_notes_attrs(struct module_notes_attrs *notes_attrs,
 			     unsigned int i)

{

kernel/module/sysfs.c +1 -1

@@ -205,7 +194,7 @@ static void add_notes_attrs(struct module *mod,
 const struct load_info *info) nattr->attr.mode = 0444;
 			nattr->size = info->sechdrs[i].sh_size;
 			nattr->private = (void
 *)info->sechdrs[i].sh_addr;
-			nattr->read = module_notes_read;
+			nattr->read = sysfs_bin_attr_simple_read;
 			++nattr;
 		}

++loaded;

Deduplicate ->read() callbacks of bin_attributes which are backed by a simple buffer in memory:

Use the newly introduced sysfs_bin_attr_simple_read() helper instead, either by referencing it directly or by declaring such bin_attributes with BIN_ATTR_SIMPLE_RO() or BIN_ATTR_SIMPLE_ADMIN_RO().

Aside from a reduction of LoC, this shaves off a few bytes from vmlinux (304 bytes on an x86_64 allyesconfig).

No functional change intended.

Signed-off-by Lukas Wunner <[email protected]>

---
 arch/powerpc/platforms/powernv/opal.c              | 10 +--------
 drivers/acpi/bgrt.c                                |  9 +-------
 drivers/firmware/dmi_scan.c                        | 12 ++--------
 drivers/firmware/efi/rci2-table.c                  | 10 +--------
 drivers/gpu/drm/i915/gvt/firmware.c                | 26 +++++-----------------
 .../intel/int340x_thermal/int3400_thermal.c        |  9 +-------
 init/initramfs.c                                   | 10 +--------
 kernel/module/sysfs.c                              | 13 +----------
 8 files changed, 14 insertions(+), 85 deletions(-)

arch/powerpc/platforms/powernv/opal.c +0 -8

index 45dd77e..5d0f35b 100644
@@ -792,14 +792,6 @@ static int __init opal_sysfs_init(void)
 	return 0;
 }
 
-static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
-				struct bin_attribute *bin_attr, char *buf,
-				loff_t off, size_t count)
-{
-	return memory_read_from_buffer(buf, count, &off, bin_attr->private,
-				       bin_attr->size);
-}
-
 static int opal_add_one_export(struct kobject *parent, const char *export_name,
 			       struct device_node *np, const char *prop_name)
 {

Deduplicate ->read() callbacks of bin_attributes which are backed by a simple buffer in memory:

Use the newly introduced sysfs_bin_attr_simple_read() helper instead, either by referencing it directly or by declaring such bin_attributes with BIN_ATTR_SIMPLE_RO() or BIN_ATTR_SIMPLE_ADMIN_RO().

Aside from a reduction of LoC, this shaves off a few bytes from vmlinux (304 bytes on an x86_64 allyesconfig).

No functional change intended.

Signed-off-by Lukas Wunner <[email protected]>

Acked-by Michael Ellerman <[email protected]> (powerpc)

---

index da79760..5193fae 100644

init/initramfs.c +0 -1

@@ -575,15 +575,7 @@ static int __init initramfs_async_setup(char *str)
 #include <linux/initrd.h>
 #include <linux/kexec.h>
 
-static ssize_t raw_read(struct file *file, struct kobject *kobj,

Hi,

Quoted 12 lines · Lukas Wunner

Deduplicate ->read() callbacks of bin_attributes which are backed by a simple buffer in memory:

Use the newly introduced sysfs_bin_attr_simple_read() helper instead, either by referencing it directly or by declaring such bin_attributes with BIN_ATTR_SIMPLE_RO() or BIN_ATTR_SIMPLE_ADMIN_RO().

Aside from a reduction of LoC, this shaves off a few bytes from vmlinux (304 bytes on an x86_64 allyesconfig).

No functional change intended.

Not really; see below.

Quoted 3 lines

Signed-off-by Lukas Wunner <[email protected]>

Acked-by Michael Ellerman <[email protected]> (powerpc)

---

...

Quoted 8 lines

index da79760..5193fae 100644

init/initramfs.c +0 -1

@@ -575,15 +575,7 @@ static int __init initramfs_async_setup(char *str)
 #include <linux/initrd.h>
 #include <linux/kexec.h>
 
-static ssize_t raw_read(struct file *file, struct kobject *kobj,

init/initramfs.c static int __init initramfs_async_setup(char *str) · line 579

Quoted 10 lines

init/initramfs.c +1 -8

-			struct bin_attribute *attr, char *buf,
-			loff_t pos, size_t count)
-{
-	memcpy(buf, attr->private + pos, count);
-	return count;
-}
-
-static BIN_ATTR(initrd, 0440, raw_read, NULL, 0);
+static BIN_ATTR(initrd, 0440, sysfs_bin_attr_simple_read, NULL, 0);
 

sysfs_bin_attr_simple_read is only declared and available if CONFIG_SYSFS=y. With m68k:m5208evb_defconfig + CONFIG_BLK_DEV_INITRD=y, this results in

/opt/buildbot/slave/qemu-m68k/build/init/initramfs.c:578:31:
error: 'sysfs_bin_attr_simple_read' undeclared here (not in a function)

This happens because CONFIG_SYSFS=n and there is no dummy function for sysfs_bin_attr_simple_read(). Presumably the problem will be seen for all configurations with CONFIG_BLK_DEV_INITRD=y and CONFIG_SYSFS=n.

Günter reports build breakage for m68k "m5208evb_defconfig" plus CONFIG_BLK_DEV_INITRD=y caused by commit 66bc1a173328 ("treewide:
Use sysfs_bin_attr_simple_read() helper").

The defconfig disables CONFIG_SYSFS, so sysfs_bin_attr_simple_read() is not compiled into the kernel. But init/initramfs.c references that function in the initializer of a struct bin_attribute.

Add an empty static inline to avoid the build breakage.

Fixes 66bc1a173328 ("treewide: Use sysfs_bin_attr_simple_read() helper")

Reported-by Guenter Roeck <[email protected]>

Closes https://lore.kernel.org/r/[email protected] open here

Signed-off-by Lukas Wunner <[email protected]>

---
 include/linux/sysfs.h | 9 +++++++++
 1 file changed, 9 insertions(+)

include/linux/sysfs.h +9 -1

index a7d725fbf739..c4e64dc11206 100644
@@ -750,6 +750,15 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
 {
        return 0;
 }
+
+static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
+                                                struct kobject *kobj,
+                                                struct bin_attribute *attr,
+                                                char *buf, loff_t off,
+                                                size_t count)
+{
+       return 0;
+}
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,
--

2.43.0

Günter reports build breakage for m68k "m5208evb_defconfig" plus CONFIG_BLK_DEV_INITRD=y caused by commit 66bc1a173328 ("treewide:
Use sysfs_bin_attr_simple_read() helper").

The defconfig disables CONFIG_SYSFS, so sysfs_bin_attr_simple_read() is not compiled into the kernel. But init/initramfs.c references that function in the initializer of a struct bin_attribute.

Add an empty static inline to avoid the build breakage.

Fixes 66bc1a173328 ("treewide: Use sysfs_bin_attr_simple_read() helper")

Reported-by Guenter Roeck <[email protected]>

Closes https://lore.kernel.org/r/[email protected] open here

Signed-off-by Lukas Wunner <[email protected]>

---
 include/linux/sysfs.h | 9 +++++++++
 1 file changed, 9 insertions(+)

include/linux/sysfs.h +9 -0

index a7d725fbf739..c4e64dc11206 100644
@@ -750,6 +750,15 @@ static inline int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
 {
 	return 0;
 }
+
+static inline ssize_t sysfs_bin_attr_simple_read(struct file *file,
+						 struct kobject *kobj,
+						 struct bin_attribute *attr,
+						 char *buf, loff_t off,
+						 size_t count)
+{
+	return 0;
+}
 #endif /* CONFIG_SYSFS */
 
 static inline int __must_check sysfs_create_file(struct kobject *kobj,

Signature · 1 line

2.43.0

For my upcoming PCI device authentication v2 patches, I have the need to expose a simple buffer in virtual memory as a bin_attribute.

It turns out we've duplicated the ->read() callback for such simple buffers a fair number of times across the tree.

So instead of reinventing the wheel, I decided to introduce a common helper and eliminate all duplications I could find.

I'm open to a bikeshedding discussion on the sysfs_bin_attr_simple_read() name. ;)

Lukas Wunner (2):
sysfs: Add sysfs_bin_attr_simple_read() helper
treewide: Use sysfs_bin_attr_simple_read() helper

 arch/powerpc/platforms/powernv/opal.c              | 10 +-------
 drivers/acpi/bgrt.c                                |  9 +-------
 drivers/firmware/dmi_scan.c                        | 12 ++--------
 drivers/firmware/efi/rci2-table.c                  | 10 +-------
 drivers/gpu/drm/i915/gvt/firmware.c                | 26 +++++----------------
 .../intel/int340x_thermal/int3400_thermal.c        |  9 +-------
 fs/sysfs/file.c                                    | 27 ++++++++++++++++++++++
 include/linux/sysfs.h                              | 15 ++++++++++++
 init/initramfs.c                                   | 10 +-------
 kernel/module/sysfs.c                              | 13 +----------
 10 files changed, 56 insertions(+), 85 deletions(-)

--
2.43.0

For my upcoming PCI device authentication v2 patches, I have the need to expose a simple buffer in virtual memory as a bin_attribute.

It turns out we've duplicated the ->read() callback for such simple buffers a fair number of times across the tree.

So instead of reinventing the wheel, I decided to introduce a common helper and eliminate all duplications I could find.

I'm open to a bikeshedding discussion on the sysfs_bin_attr_simple_read() name. ;)

Lukas Wunner (2):
sysfs: Add sysfs_bin_attr_simple_read() helper
treewide: Use sysfs_bin_attr_simple_read() helper

 arch/powerpc/platforms/powernv/opal.c              | 10 +-------
 drivers/acpi/bgrt.c                                |  9 +-------
 drivers/firmware/dmi_scan.c                        | 12 ++--------
 drivers/firmware/efi/rci2-table.c                  | 10 +-------
 drivers/gpu/drm/i915/gvt/firmware.c                | 26 +++++----------------
 .../intel/int340x_thermal/int3400_thermal.c        |  9 +-------
 fs/sysfs/file.c                                    | 27 ++++++++++++++++++++++
 include/linux/sysfs.h                              | 15 ++++++++++++
 init/initramfs.c                                   | 10 +-------
 kernel/module/sysfs.c                              | 13 +----------
 10 files changed, 56 insertions(+), 85 deletions(-)

--

For my upcoming PCI device authentication v2 patches, I have the need to expose a simple buffer in virtual memory as a bin_attribute.

It turns out we've duplicated the ->read() callback for such simple buffers a fair number of times across the tree.

So instead of reinventing the wheel, I decided to introduce a common helper and eliminate all duplications I could find.

I'm open to a bikeshedding discussion on the sysfs_bin_attr_simple_read() name. ;)

Quoted 6 lines · Lukas Wunner

For my upcoming PCI device authentication v2 patches, I have the need to expose a simple buffer in virtual memory as a bin_attribute.

It turns out we've duplicated the ->read() callback for such simple buffers a fair number of times across the tree.

Quoted 5 lines

So instead of reinventing the wheel, I decided to introduce a common helper and eliminate all duplications I could find.

I'm open to a bikeshedding discussion on the sysfs_bin_attr_simple_read() name. ;)

Seems like no one objects, should I just take this through my driver-core tree for 6.10?