[PATCH v2 0/4] blk-integrity: drop integrity_kobj from gendisk

[PATCH v2 0/4] blk-integrity: drop integrity_kobj from gendisk

[Thomas Weißschuh]

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 <linux@weissschuh.net>
---
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 <linux@weissschuh.net>

[PATCH v2 1/4] blk-integrity: use sysfs_emit

[Thomas Weißschuh]

The correct way to emit data into sysfs is via sysfs_emit(), use it.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 block/blk-integrity.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index 8f01d786f5cb..5dd820ee4d1c 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -248,20 +248,20 @@ static ssize_t integrity_attr_store(struct kobject *kobj,
 static ssize_t integrity_format_show(struct blk_integrity *bi, char *page)
 {
 	if (bi->profile && bi->profile->name)
-		return sprintf(page, "%s\n", bi->profile->name);
+		return sysfs_emit(page, "%s\n", bi->profile->name);
 	else
-		return sprintf(page, "none\n");
+		return sysfs_emit(page, "none\n");
 }
 
 static ssize_t integrity_tag_size_show(struct blk_integrity *bi, char *page)
 {
-	return sprintf(page, "%u\n", bi->tag_size);
+	return sysfs_emit(page, "%u\n", bi->tag_size);
 }
 
 static ssize_t integrity_interval_show(struct blk_integrity *bi, char *page)
 {
-	return sprintf(page, "%u\n",
-		       bi->interval_exp ? 1 << bi->interval_exp : 0);
+	return sysfs_emit(page, "%u\n",
+			  bi->interval_exp ? 1 << bi->interval_exp : 0);
 }
 
 static ssize_t integrity_verify_store(struct blk_integrity *bi,
@@ -280,7 +280,7 @@ static ssize_t integrity_verify_store(struct blk_integrity *bi,
 
 static ssize_t integrity_verify_show(struct blk_integrity *bi, char *page)
 {
-	return sprintf(page, "%d\n", (bi->flags & BLK_INTEGRITY_VERIFY) != 0);
+	return sysfs_emit(page, "%d\n", (bi->flags & BLK_INTEGRITY_VERIFY) != 0);
 }
 
 static ssize_t integrity_generate_store(struct blk_integrity *bi,
@@ -299,13 +299,13 @@ static ssize_t integrity_generate_store(struct blk_integrity *bi,
 
 static ssize_t integrity_generate_show(struct blk_integrity *bi, char *page)
 {
-	return sprintf(page, "%d\n", (bi->flags & BLK_INTEGRITY_GENERATE) != 0);
+	return sysfs_emit(page, "%d\n", (bi->flags & BLK_INTEGRITY_GENERATE) != 0);
 }
 
 static ssize_t integrity_device_show(struct blk_integrity *bi, char *page)
 {
-	return sprintf(page, "%u\n",
-		       (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE) != 0);
+	return sysfs_emit(page, "%u\n",
+			  (bi->flags & BLK_INTEGRITY_DEVICE_CAPABLE) != 0);
 }
 
 static struct integrity_sysfs_entry integrity_format_entry = {

-- 
2.39.2

Re: [PATCH v2 1/4] blk-integrity: use sysfs_emit

[Christoph Hellwig]

>  	if (bi->profile && bi->profile->name)
> -		return sprintf(page, "%s\n", bi->profile->name);
> +		return sysfs_emit(page, "%s\n", bi->profile->name);
>  	else

Might be worth to drop the else here if you touch the function.

>
> +	return sysfs_emit(page, "%d\n", (bi->flags & BLK_INTEGRITY_VERIFY) != 0);

Please shorten these != 0 to !! expressions, i.e.

	return sysfs_emit(page, "%d\n", !!(bi->flags & BLK_INTEGRITY_VERIFY));

[PATCH v2 2/4] blk-integrity: convert to struct device_attribute

[Thomas Weißschuh]

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 <linux@weissschuh.net>
---
 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

Re: [PATCH v2 2/4] blk-integrity: convert to struct device_attribute

[Christoph Hellwig]

> +		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.

Re: [PATCH v2 2/4] blk-integrity: convert to struct device_attribute

[Thomas Weißschuh]

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.

[PATCH v2 3/4] blk-integrity: register sysfs attributes on struct device

[Thomas Weißschuh]

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 <linux@weissschuh.net>
---
 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

Re: [PATCH v2 3/4] blk-integrity: register sysfs attributes on struct device

[Christoph Hellwig]

> +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?

Re: [PATCH v2 3/4] blk-integrity: register sysfs attributes on struct device

[Thomas Weißschuh]

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.

[PATCH v2 4/4] blk-integrity: drop integrity_kobj from gendisk

[Thomas Weißschuh]

The previous patches made the integrity_kobj member in struct gendisk
superfluous, remove it.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/blkdev.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index d1aee08f8c18..b5bc85c3166f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -163,9 +163,6 @@ struct gendisk {
 	struct timer_rand_state *random;
 	atomic_t sync_io;		/* RAID */
 	struct disk_events *ev;
-#ifdef  CONFIG_BLK_DEV_INTEGRITY
-	struct kobject integrity_kobj;
-#endif	/* CONFIG_BLK_DEV_INTEGRITY */
 
 #ifdef CONFIG_BLK_DEV_ZONED
 	/*

-- 
2.39.2

Re: [PATCH v2 4/4] blk-integrity: drop integrity_kobj from gendisk

[Christoph Hellwig]

On Fri, Mar 10, 2023 at 10:40:05PM +0000, Thomas Weißschuh wrote:
> The previous patches made the integrity_kobj member in struct gendisk
> superfluous, remove it.

Maybe fold this into the patch that removes the last use?