The embedded member integrity_kobj member of struct gendisk violates
the assumption of the driver core that only one struct kobject should be
embedded into another object and then manages its lifetime.
As the integrity_kobj is only used to hold a few sysfs attributes it can
be replaced by direct device_attributes and removed.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
Changes in v2:
- Get rid of integrity_kobj completely
- Migrate to sysfs_emit helper
- Link to v1: https://lore.kernel.org/r/20230309-kobj_release-gendisk_integrity-v1-1-a240f54eac60@weissschuh.net
---
Thomas Weißschuh (4):
blk-integrity: use sysfs_emit
blk-integrity: convert to struct device_attribute
blk-integrity: register sysfs attributes on struct device
blk-integrity: drop integrity_kobj from gendisk
block/blk-integrity.c | 159 +++++++++++++++++--------------------------------
include/linux/blkdev.h | 3 -
2 files changed, 55 insertions(+), 107 deletions(-)
---
base-commit: 55a21105ecc156495446d8ae75d7d73f66baed7b
change-id: 20230309-kobj_release-gendisk_integrity-e26c0bc126aa
Best regards,
--
Thomas Weißschuh <[email protected]>
An upcoming patch will register the integrity attributes directly with
the struct device kobject.
For this the attributes have to be implemented in terms of
struct device_attribute.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
block/blk-integrity.c | 117 +++++++++++++++++++++++---------------------------
1 file changed, 54 insertions(+), 63 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 5dd820ee4d1c..e1c3e3591c82 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -212,21 +212,15 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
return true;
}
-struct integrity_sysfs_entry {
- struct attribute attr;
- ssize_t (*show)(struct blk_integrity *, char *);
- ssize_t (*store)(struct blk_integrity *, const char *, size_t);
-};
-
static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
char *page)
{
struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct blk_integrity *bi = &disk->queue->integrity;
- struct integrity_sysfs_entry *entry =
- container_of(attr, struct integrity_sysfs_entry, attr);
+ struct device *dev = disk_to_dev(disk);
+ struct device_attribute *dev_attr =
+ container_of(attr, struct device_attribute, attr);
- return entry->show(bi, page);
+ return dev_attr->show(dev, dev_attr, page);
}
static ssize_t integrity_attr_store(struct kobject *kobj,
@@ -234,39 +228,52 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
size_t count)
{
struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct blk_integrity *bi = &disk->queue->integrity;
- struct integrity_sysfs_entry *entry =
- container_of(attr, struct integrity_sysfs_entry, attr);
+ struct device *dev = disk_to_dev(disk);
+ struct device_attribute *dev_attr =
+ container_of(attr, struct device_attribute, attr);
ssize_t ret = 0;
- if (entry->store)
- ret = entry->store(bi, page, count);
+ if (dev_attr->store)
+ ret = dev_attr->store(dev, dev_attr, page, count);
return ret;
}
-static ssize_t integrity_format_show(struct blk_integrity *bi, char *page)
+static inline struct blk_integrity *dev_to_bi(struct device *dev)
+{
+ return &dev_to_disk(dev)->queue->integrity;
+}
+
+static ssize_t format_show(struct device *dev, struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
if (bi->profile && bi->profile->name)
return sysfs_emit(page, "%s\n", bi->profile->name);
else
return sysfs_emit(page, "none\n");
}
-static ssize_t integrity_tag_size_show(struct blk_integrity *bi, char *page)
+static ssize_t tag_size_show(struct device *dev, struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%u\n", bi->tag_size);
}
-static ssize_t integrity_interval_show(struct blk_integrity *bi, char *page)
+static ssize_t protection_interval_bytes_show(struct device *dev,
+ struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%u\n",
bi->interval_exp ? 1 << bi->interval_exp : 0);
}
-static ssize_t integrity_verify_store(struct blk_integrity *bi,
- const char *page, size_t count)
+static ssize_t read_verify_store(struct device *dev, struct device_attribute *attr,
+ const char *page, size_t count)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
char *p = (char *) page;
unsigned long val = simple_strtoul(p, &p, 10);
@@ -278,14 +285,18 @@ static ssize_t integrity_verify_store(struct blk_integrity *bi,
return count;
}
-static ssize_t integrity_verify_show(struct blk_integrity *bi, char *page)
+static ssize_t read_verify_show(struct device *dev, struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%d\n", (bi->flags & BLK_INTEGRITY_VERIFY) != 0);
}
-static ssize_t integrity_generate_store(struct blk_integrity *bi,
- const char *page, size_t count)
+static ssize_t write_generate_store(struct device *dev, struct device_attribute *attr,
+ const char *page, size_t count)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
char *p = (char *) page;
unsigned long val = simple_strtoul(p, &p, 10);
@@ -297,57 +308,37 @@ static ssize_t integrity_generate_store(struct blk_integrity *bi,
return count;
}
-static ssize_t integrity_generate_show(struct blk_integrity *bi, char *page)
+static ssize_t write_generate_show(struct device *dev, struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%d\n", (bi->flags & BLK_INTEGRITY_GENERATE) != 0);
}
-static ssize_t integrity_device_show(struct blk_integrity *bi, char *page)
+static ssize_t device_is_integrity_capable_show(struct device *dev,
+ struct device_attribute *attr, char *page)
{
+ struct blk_integrity *bi = dev_to_bi(dev);
+
return sysfs_emit(page, "%u\n",
(bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE) != 0);
}
-static struct integrity_sysfs_entry integrity_format_entry = {
- .attr = { .name = "format", .mode = 0444 },
- .show = integrity_format_show,
-};
-
-static struct integrity_sysfs_entry integrity_tag_size_entry = {
- .attr = { .name = "tag_size", .mode = 0444 },
- .show = integrity_tag_size_show,
-};
-
-static struct integrity_sysfs_entry integrity_interval_entry = {
- .attr = { .name = "protection_interval_bytes", .mode = 0444 },
- .show = integrity_interval_show,
-};
-
-static struct integrity_sysfs_entry integrity_verify_entry = {
- .attr = { .name = "read_verify", .mode = 0644 },
- .show = integrity_verify_show,
- .store = integrity_verify_store,
-};
-
-static struct integrity_sysfs_entry integrity_generate_entry = {
- .attr = { .name = "write_generate", .mode = 0644 },
- .show = integrity_generate_show,
- .store = integrity_generate_store,
-};
-
-static struct integrity_sysfs_entry integrity_device_entry = {
- .attr = { .name = "device_is_integrity_capable", .mode = 0444 },
- .show = integrity_device_show,
-};
+static DEVICE_ATTR_RO(format);
+static DEVICE_ATTR_RO(tag_size);
+static DEVICE_ATTR_RO(protection_interval_bytes);
+static DEVICE_ATTR_RW(read_verify);
+static DEVICE_ATTR_RW(write_generate);
+static DEVICE_ATTR_RO(device_is_integrity_capable);
static struct attribute *integrity_attrs[] = {
- &integrity_format_entry.attr,
- &integrity_tag_size_entry.attr,
- &integrity_interval_entry.attr,
- &integrity_verify_entry.attr,
- &integrity_generate_entry.attr,
- &integrity_device_entry.attr,
- NULL,
+ &dev_attr_format.attr,
+ &dev_attr_tag_size.attr,
+ &dev_attr_protection_interval_bytes.attr,
+ &dev_attr_read_verify.attr,
+ &dev_attr_write_generate.attr,
+ &dev_attr_device_is_integrity_capable.attr,
+ NULL
};
ATTRIBUTE_GROUPS(integrity);
--
2.39.2
The "integrity" kobject only acted as a holder for static sysfs entries.
It also was embedded into struct gendisk without managing it, violating
assumptions of the driver core.
Instead register the sysfs entries directly onto the struct device.
Signed-off-by: Thomas Weißschuh <[email protected]>
---
block/blk-integrity.c | 50 +++++---------------------------------------------
1 file changed, 5 insertions(+), 45 deletions(-)
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index e1c3e3591c82..79eb21482036 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -212,33 +212,6 @@ bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
return true;
}
-static ssize_t integrity_attr_show(struct kobject *kobj, struct attribute *attr,
- char *page)
-{
- struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct device *dev = disk_to_dev(disk);
- struct device_attribute *dev_attr =
- container_of(attr, struct device_attribute, attr);
-
- return dev_attr->show(dev, dev_attr, page);
-}
-
-static ssize_t integrity_attr_store(struct kobject *kobj,
- struct attribute *attr, const char *page,
- size_t count)
-{
- struct gendisk *disk = container_of(kobj, struct gendisk, integrity_kobj);
- struct device *dev = disk_to_dev(disk);
- struct device_attribute *dev_attr =
- container_of(attr, struct device_attribute, attr);
- ssize_t ret = 0;
-
- if (dev_attr->store)
- ret = dev_attr->store(dev, dev_attr, page, count);
-
- return ret;
-}
-
static inline struct blk_integrity *dev_to_bi(struct device *dev)
{
return &dev_to_disk(dev)->queue->integrity;
@@ -340,17 +313,12 @@ static struct attribute *integrity_attrs[] = {
&dev_attr_device_is_integrity_capable.attr,
NULL
};
-ATTRIBUTE_GROUPS(integrity);
-static const struct sysfs_ops integrity_ops = {
- .show = &integrity_attr_show,
- .store = &integrity_attr_store,
+static const struct attribute_group integrity_group = {
+ .name = "integrity", .attrs = integrity_attrs,
};
-static const struct kobj_type integrity_ktype = {
- .default_groups = integrity_groups,
- .sysfs_ops = &integrity_ops,
-};
+__ATTRIBUTE_GROUPS(integrity);
static blk_status_t blk_integrity_nop_fn(struct blk_integrity_iter *iter)
{
@@ -431,18 +399,10 @@ EXPORT_SYMBOL(blk_integrity_unregister);
int blk_integrity_add(struct gendisk *disk)
{
- int ret;
-
- ret = kobject_init_and_add(&disk->integrity_kobj, &integrity_ktype,
- &disk_to_dev(disk)->kobj, "%s", "integrity");
- if (!ret)
- kobject_uevent(&disk->integrity_kobj, KOBJ_ADD);
- return ret;
+ return device_add_groups(disk_to_dev(disk), integrity_groups);
}
void blk_integrity_del(struct gendisk *disk)
{
- kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
- kobject_del(&disk->integrity_kobj);
- kobject_put(&disk->integrity_kobj);
+ device_remove_groups(disk_to_dev(disk), integrity_groups);
}
--
2.39.2
> + container_of(attr, struct device_attribute, attr);
> ssize_t ret = 0;
>
> + if (dev_attr->store)
> + ret = dev_attr->store(dev, dev_attr, page, count);
>
> return ret;
This can be simplified to:
if (!rev_attr->store)
return 0;
return dev_attr->store(dev, dev_attr, page, count);
(I'm still confused why 0 is the right return value here, but that's not
new in your patch, so better don't rock that boat).
> +static ssize_t format_show(struct device *dev, struct device_attribute *attr, char *page)
Please avoid the overly long line here. an in the other methods.
> +static const struct attribute_group integrity_group = {
Double whitespace before the =
> + .name = "integrity", .attrs = integrity_attrs,
> };
We generally put each field member on separate lines for readability.
> int blk_integrity_add(struct gendisk *disk)
> {
> + return device_add_groups(disk_to_dev(disk), integrity_groups);
> }
>
> void blk_integrity_del(struct gendisk *disk)
> {
> + device_remove_groups(disk_to_dev(disk), integrity_groups);
Can't we just add integrity_group to disk_attr_groups and remove these
calls entirely?
On Wed, Mar 15, 2023 at 08:03:00AM -0700, Christoph Hellwig wrote:
> > + container_of(attr, struct device_attribute, attr);
> > ssize_t ret = 0;
> >
> > + if (dev_attr->store)
> > + ret = dev_attr->store(dev, dev_attr, page, count);
> >
> > return ret;
>
> This can be simplified to:
>
> if (!rev_attr->store)
> return 0;
> return dev_attr->store(dev, dev_attr, page, count);
>
> (I'm still confused why 0 is the right return value here, but that's not
> new in your patch, so better don't rock that boat).
This indeed looks weird.
Please note that this case will become -EIO in the next patch switching
over to the standard dev_sysfs_ops.
It shouldn't matter though as all writable attributes all have the
store() handler defined.
> > +static ssize_t format_show(struct device *dev, struct device_attribute *attr, char *page)
>
> Please avoid the overly long line here. an in the other methods.
This was following the "new" 100-characters per line limit.
The new revision will follow the limit with 80.
On Wed, Mar 15, 2023 at 08:06:51AM -0700, Christoph Hellwig wrote:
> > +static const struct attribute_group integrity_group = {
>
> Double whitespace before the =
Ack.
> > + .name = "integrity", .attrs = integrity_attrs,
> > };
>
> We generally put each field member on separate lines for readability.
Ack.
> > int blk_integrity_add(struct gendisk *disk)
> > {
> > + return device_add_groups(disk_to_dev(disk), integrity_groups);
> > }
> >
> > void blk_integrity_del(struct gendisk *disk)
> > {
> > + device_remove_groups(disk_to_dev(disk), integrity_groups);
>
> Can't we just add integrity_group to disk_attr_groups and remove these
> calls entirely?
Thanks for the pointer. This works and is indeed nicer.