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
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(+)
diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
index 6b7652f..289b57d 100644
--- a/fs/sysfs/file.c
+++ b/fs/sysfs/file.c
@@ -783,3 +783,30 @@ int sysfs_emit_at(char *buf, int at, const char *fmt, ...)
return len;
}
EXPORT_SYMBOL_GPL(sysfs_emit_at);
+
+/**
+ * sysfs_bin_attr_simple_read - read callback to simply copy from memory.
+ * @file: attribute file which is being read.
+ * @kobj: object to which the attribute belongs.
+ * @attr: attribute descriptor.
+ * @buf: destination buffer.
+ * @off: offset in bytes from which to read.
+ * @count: maximum number of bytes to read.
+ *
+ * Simple ->read() callback for bin_attributes backed by a buffer in memory.
+ * The @private and @size members in struct bin_attribute must be set to the
+ * buffer's location and size before the bin_attribute is created in sysfs.
+ *
+ * Bounds check for @off and @count is done in sysfs_kf_bin_read().
+ * Negative value check for @off is done in vfs_setpos() and default_llseek().
+ *
+ * Returns number of bytes written to @buf.
+ */
+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)
+{
+ memcpy(buf, attr->private + off, count);
+ return count;
+}
+EXPORT_SYMBOL_GPL(sysfs_bin_attr_simple_read);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 326341c..a7d725f 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -371,6 +371,17 @@ struct bin_attribute bin_attr_##_name = __BIN_ATTR_ADMIN_RO(_name, _size)
#define BIN_ATTR_ADMIN_RW(_name, _size) \
struct bin_attribute bin_attr_##_name = __BIN_ATTR_ADMIN_RW(_name, _size)
+#define __BIN_ATTR_SIMPLE_RO(_name, _mode) { \
+ .attr = { .name = __stringify(_name), .mode = _mode }, \
+ .read = sysfs_bin_attr_simple_read, \
+}
+
+#define BIN_ATTR_SIMPLE_RO(_name) \
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_SIMPLE_RO(_name, 0444)
+
+#define BIN_ATTR_SIMPLE_ADMIN_RO(_name) \
+struct bin_attribute bin_attr_##_name = __BIN_ATTR_SIMPLE_RO(_name, 0400)
+
struct sysfs_ops {
ssize_t (*show)(struct kobject *, struct attribute *, char *);
ssize_t (*store)(struct kobject *, struct attribute *, const char *, size_t);
@@ -478,6 +489,10 @@ int sysfs_group_change_owner(struct kobject *kobj,
__printf(3, 4)
int sysfs_emit_at(char *buf, int at, const char *fmt, ...);
+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);
+
#else /* CONFIG_SYSFS */
static inline int sysfs_create_dir_ns(struct kobject *kobj, const void *ns)
--
2.43.0
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(-)
diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
index 45dd77e..5d0f35b 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -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)
{
@@ -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];
diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
index e4fb9e2..d1d9c92 100644
--- a/drivers/acpi/bgrt.c
+++ b/drivers/acpi/bgrt.c
@@ -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,
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 015c95a..3d0f773 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -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)
{
diff --git a/drivers/firmware/efi/rci2-table.c b/drivers/firmware/efi/rci2-table.c
index de1a9a1..4fd45d6 100644
--- a/drivers/firmware/efi/rci2-table.c
+++ b/drivers/firmware/efi/rci2-table.c
@@ -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)
{
diff --git a/drivers/gpu/drm/i915/gvt/firmware.c b/drivers/gpu/drm/i915/gvt/firmware.c
index 4dd52ac..5e66a26 100644
--- a/drivers/gpu/drm/i915/gvt/firmware.c
+++ b/drivers/gpu/drm/i915/gvt/firmware.c
@@ -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);
- 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) {
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);
}
/**
diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c 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,
- struct bin_attribute *attr, char *buf, loff_t off, size_t count)
-{
- 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,
diff --git a/init/initramfs.c b/init/initramfs.c
index da79760..5193fae 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -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)
{
diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
index d964167..26efe13 100644
--- a/kernel/module/sysfs.c
+++ b/kernel/module/sysfs.c
@@ -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)
{
@@ -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;
--
2.43.0
On Sat, 6 Apr 2024 at 15:52, Lukas Wunner <[email protected]> wrote:
>
> 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
>
Acked-by: Ard Biesheuvel <[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 +-------
> 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
>
On Sat, 6 Apr 2024 15:52:02 +0200
Lukas Wunner <[email protected]> wrote:
> 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.
>
As for GVT, looks good to me.
Acked-by: Zhi Wang <[email protected]>
> 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(-)
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c
> b/arch/powerpc/platforms/powernv/opal.c index 45dd77e..5d0f35b 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -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)
> {
> @@ -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];
>
> diff --git a/drivers/acpi/bgrt.c b/drivers/acpi/bgrt.c
> index e4fb9e2..d1d9c92 100644
> --- a/drivers/acpi/bgrt.c
> +++ b/drivers/acpi/bgrt.c
> @@ -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,
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index 015c95a..3d0f773 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -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)
> {
> diff --git a/drivers/firmware/efi/rci2-table.c
> b/drivers/firmware/efi/rci2-table.c index de1a9a1..4fd45d6 100644
> --- a/drivers/firmware/efi/rci2-table.c
> +++ b/drivers/firmware/efi/rci2-table.c
> @@ -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)
> {
> diff --git a/drivers/gpu/drm/i915/gvt/firmware.c
> b/drivers/gpu/drm/i915/gvt/firmware.c index 4dd52ac..5e66a26 100644
> --- a/drivers/gpu/drm/i915/gvt/firmware.c
> +++ b/drivers/gpu/drm/i915/gvt/firmware.c
> @@ -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);
> - 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) {
> 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);
> }
>
> /**
> diff --git a/drivers/thermal/intel/int340x_thermal/int3400_thermal.c
> 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,
> - struct bin_attribute *attr, char *buf, loff_t off,
> size_t count) -{
> - 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,
> diff --git a/init/initramfs.c b/init/initramfs.c
> index da79760..5193fae 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -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)
> {
> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
> index d964167..26efe13 100644
> --- a/kernel/module/sysfs.c
> +++ b/kernel/module/sysfs.c
> @@ -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)
> {
> @@ -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;
Lukas Wunner <[email protected]> writes:
> 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(-)
>
> diff --git a/arch/powerpc/platforms/powernv/opal.c b/arch/powerpc/platforms/powernv/opal.c
> index 45dd77e..5d0f35b 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -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)
> {
> @@ -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];
I gave it a quick boot and checked I could still read the attributes,
everything seems fine.
Acked-by: Michael Ellerman <[email protected]> (powerpc)
cheers
On Sat, Apr 6, 2024 at 3:52 PM Lukas Wunner <[email protected]> wrote:
>
> 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 the series
Acked-by: Rafael J. Wysocki <[email protected]>
On Sat, Apr 06, 2024 at 03:52:01PM +0200, Lukas Wunner wrote:
> 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(+)
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Sat, Apr 06, 2024 at 03:52:00PM +0200, Lukas Wunner wrote:
> 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. ;)
Seems like no one objects, should I just take this through my
driver-core tree for 6.10?
thanks,
greg k-h
On Thu, Apr 11, 2024 at 03:07:46PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 06, 2024 at 03:52:00PM +0200, Lukas Wunner wrote:
> > 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. ;)
>
> Seems like no one objects, should I just take this through my
> driver-core tree for 6.10?
That would be awesome, thank you!
Lukas
Hi,
On Sat, Apr 06, 2024 at 03:52:02PM +0200, Lukas Wunner wrote:
> 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.
> Signed-off-by: Lukas Wunner <[email protected]>
> Acked-by: Michael Ellerman <[email protected]> (powerpc)
> ---
..
> index da79760..5193fae 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -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);
>
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.
On a side note, init/initramfs.c does not directly include linux/sysfs.h.
I don't know if that might cause problems with other builds.
Guenter
On Wed, May 22, 2024 at 07:51:35PM -0700, Guenter Roeck wrote:
> Hi,
>
> On Sat, Apr 06, 2024 at 03:52:02PM +0200, Lukas Wunner wrote:
> > 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.
>
> > Signed-off-by: Lukas Wunner <[email protected]>
> > Acked-by: Michael Ellerman <[email protected]> (powerpc)
> > ---
> ...
> > index da79760..5193fae 100644
> > --- a/init/initramfs.c
> > +++ b/init/initramfs.c
> > @@ -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);
> >
>
> 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.
Lukas, can you send a patch adding a dummy function?
thanks,
greg k-h
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]
Signed-off-by: Lukas Wunner <[email protected]>
---
include/linux/sysfs.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index a7d725fbf739..c4e64dc11206 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -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
On Thu, May 23, 2024 at 1:00 PM Lukas Wunner <[email protected]> wrote:
>
> 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]
> Signed-off-by: Lukas Wunner <[email protected]>
Works for me.
Reviewed-by: Rafael J. Wysocki <[email protected]>
> ---
> include/linux/sysfs.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index a7d725fbf739..c4e64dc11206 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -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
>
On Thu, May 23, 2024 at 01:00:00PM +0200, Lukas Wunner wrote:
> 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]
> Signed-off-by: Lukas Wunner <[email protected]>
Tested-by: Guenter Roeck <[email protected]>
> ---
> include/linux/sysfs.h | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index a7d725fbf739..c4e64dc11206 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -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
>