| Message ID | 20211026014240.4098365-1-maier@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v3] scsi: core: Fix early registration of sysfs attributes for scsi_device | expand |
On Tue, Oct 26, 2021 at 03:42:40AM +0200, Steffen Maier wrote: > v4.17 commit 86b87cde0b55 ("scsi: core: host template attribute groups") > introduced explicit sysfs_create_groups() in scsi_sysfs_add_sdev() > and sysfs_remove_groups() in __scsi_remove_device(), both for sdev_gendev, > based on a new field const struct attribute_group **sdev_groups > of struct scsi_host_template. > > Commit 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") > removed above explicit (de)registration of scsi_device attribute groups. > It also converted all scsi_device attributes and attribute_groups to > end up in a new field const struct attribute_group *gendev_attr_groups[6] > of struct scsi_device. However, that new field was not used anywhere. Oh.. damn, I didn't notice either. > > Surprisingly, this only caused missing LLDD specific scsi_device sysfs > attributes. Whereas, scsi core attributes from scsi_sdev_attr_groups > did continue to exist because of scsi_dev_type.groups. Hmm, maybe this is out of scope for this fix, but couldn't we essentially do the same thing for the host attributes. Have the `shost_class` take the `scsi_shost_attr_group` as default attributes for the shost class, and then assign the `shost_groups` from the LLDD template to `shost_dev.groups` as optional attributes? Then we get rid of the indirection loop in `hosts.c` as well, that was introduce with he original patch by Bart. Just a shot in the dark, I don't know whether the `struct class` behaves the same in this case as `struct device_type`. > > We separate scsi core attibutes from LLDD specific attributes. > Hence, we keep the initializing assignment scsi_dev_type = > { .groups = scsi_sdev_attr_groups, } as this takes care of core > attributes. Without the separation, it would cause attribute double > registration due to scsi_dev_type.groups and sdev_gendev.groups. > > Julian suggested to assign the sdev_groups pointer of the > scsi_host_template directly to the groups pointer of sdev_gendev. > This way we can delete the container scsi_device.gendev_attr_groups > and the loop copying each entry from hostt->sdev_groups to > sdev->gendev_attr_groups. > > Alternative approaches ruled out: > Assigning gendev_attr_groups to sdev_dev has no visible effect. > Assigning sdev->gendev_attr_groups to scsi_dev_type.groups > caused scsi_device of all scsi host types to get LLDD specific > attributes of the LLDD for which the last sdev alloc happened to occur, > as that overwrote scsi_dev_type.groups, > e.g. scsi_debug had zfcp-specific scsi_device attributes. > > Signed-off-by: Steffen Maier <maier@linux.ibm.com> > Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") > Suggested-by: Julian Wiedmann <jwi@linux.ibm.com>
On 10/25/21 6:42 PM, Steffen Maier wrote: > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index c26f0e29e8cd..fa064bf9a55c 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1571,7 +1571,6 @@ static struct device_type scsi_dev_type = { > > void scsi_sysfs_device_initialize(struct scsi_device *sdev) > { > - int i, j = 0; > unsigned long flags; > struct Scsi_Host *shost = sdev->host; > struct scsi_host_template *hostt = shost->hostt; > @@ -1583,15 +1582,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) > scsi_enable_async_suspend(&sdev->sdev_gendev); > dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu", > sdev->host->host_no, sdev->channel, sdev->id, sdev->lun); > - sdev->gendev_attr_groups[j++] = &scsi_sdev_attr_group; > - if (hostt->sdev_groups) { > - for (i = 0; hostt->sdev_groups[i] && > - j < ARRAY_SIZE(sdev->gendev_attr_groups); > - i++, j++) { > - sdev->gendev_attr_groups[j] = hostt->sdev_groups[i]; > - } > - } > - WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); > + sdev->sdev_gendev.groups = hostt->sdev_groups; > > device_initialize(&sdev->sdev_dev); > sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev); > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index b1e9b3bd3a60..b97e142a7ca9 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -225,12 +225,6 @@ struct scsi_device { > > struct device sdev_gendev, > sdev_dev; > - /* > - * The array size 6 provides space for one attribute group for the > - * SCSI core, four attribute groups defined by SCSI LLDs and one > - * terminating NULL pointer. > - */ > - const struct attribute_group *gendev_attr_groups[6]; > > struct execute_work ew; /* used to get process context on put */ > struct work_struct requeue_work; Thanks! Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On 10/26/21 3:00 AM, Benjamin Block wrote: > Hmm, maybe this is out of scope for this fix, but couldn't we > essentially do the same thing for the host attributes. Have the > `shost_class` take the `scsi_shost_attr_group` as default attributes for > the shost class, and then assign the `shost_groups` from the LLDD > template to `shost_dev.groups` as optional attributes? > > Then we get rid of the indirection loop in `hosts.c` as well, that was > introduce with he original patch by Bart. > > Just a shot in the dark, I don't know whether the `struct class` behaves > the same in this case as `struct device_type`. Is something like this what you have in mind? Thanks, Bart. Subject: [PATCH] scsi: core: Remove Scsi_Host.shost_dev_attr_groups Suggested-by: Benjamin Block <bblock@linux.ibm.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/hosts.c | 15 +++------------ drivers/scsi/scsi_priv.h | 2 +- drivers/scsi/scsi_sysfs.c | 7 ++++++- include/scsi/scsi_host.h | 6 ------ 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c index 17aef936bc90..f88e7ed77dbb 100644 --- a/drivers/scsi/hosts.c +++ b/drivers/scsi/hosts.c @@ -61,6 +61,7 @@ static void scsi_host_cls_release(struct device *dev) static struct class shost_class = { .name = "scsi_host", .dev_release = scsi_host_cls_release, + .dev_groups = scsi_shost_groups, }; /** @@ -376,7 +377,7 @@ static struct device_type scsi_host_type = { struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) { struct Scsi_Host *shost; - int index, i, j = 0; + int index; shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); if (!shost) @@ -481,17 +482,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) shost->shost_dev.parent = &shost->shost_gendev; shost->shost_dev.class = &shost_class; dev_set_name(&shost->shost_dev, "host%d", shost->host_no); - shost->shost_dev.groups = shost->shost_dev_attr_groups; - shost->shost_dev_attr_groups[j++] = &scsi_shost_attr_group; - if (sht->shost_groups) { - for (i = 0; sht->shost_groups[i] && - j < ARRAY_SIZE(shost->shost_dev_attr_groups); - i++, j++) { - shost->shost_dev_attr_groups[j] = - sht->shost_groups[i]; - } - } - WARN_ON_ONCE(j >= ARRAY_SIZE(shost->shost_dev_attr_groups)); + shost->shost_dev.groups = sht->shost_groups; shost->ehandler = kthread_run(scsi_error_handler, shost, "scsi_eh_%d", shost->host_no); diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h index a278fc8948f4..0f5743f4769b 100644 --- a/drivers/scsi/scsi_priv.h +++ b/drivers/scsi/scsi_priv.h @@ -144,7 +144,7 @@ extern struct scsi_transport_template blank_transport_template; extern void __scsi_remove_device(struct scsi_device *); extern struct bus_type scsi_bus_type; -extern const struct attribute_group scsi_shost_attr_group; +extern const struct attribute_group *scsi_shost_groups[]; /* scsi_netlink.c */ #ifdef CONFIG_SCSI_NETLINK diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index c26f0e29e8cd..f360154b5241 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -424,10 +424,15 @@ static struct attribute *scsi_sysfs_shost_attrs[] = { NULL }; -const struct attribute_group scsi_shost_attr_group = { +static const struct attribute_group scsi_shost_attr_group = { .attrs = scsi_sysfs_shost_attrs, }; +const struct attribute_group *scsi_shost_groups[] = { + &scsi_shost_attr_group, + NULL +}; + static void scsi_device_cls_release(struct device *class_dev) { struct scsi_device *sdev; diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index ae715959f886..97cdad14de56 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -690,12 +690,6 @@ struct Scsi_Host { /* ldm bits */ struct device shost_gendev, shost_dev; - /* - * The array size 3 provides space for one attribute group defined by - * the SCSI core, one attribute group defined by the SCSI LLD and one - * terminating NULL pointer. - */ - const struct attribute_group *shost_dev_attr_groups[3]; /* * Points to the transport data (if any) which is allocated
On Tue, Oct 26, 2021 at 02:48:55PM -0700, Bart Van Assche wrote: > On 10/26/21 3:00 AM, Benjamin Block wrote: > > Hmm, maybe this is out of scope for this fix, but couldn't we > > essentially do the same thing for the host attributes. Have the > > `shost_class` take the `scsi_shost_attr_group` as default attributes for > > the shost class, and then assign the `shost_groups` from the LLDD > > template to `shost_dev.groups` as optional attributes? > > > > Then we get rid of the indirection loop in `hosts.c` as well, that was > > introduce with he original patch by Bart. > > > > Just a shot in the dark, I don't know whether the `struct class` behaves > > the same in this case as `struct device_type`. > > Is something like this what you have in mind? Yes, exactly. That's what I meant. That make host and dev similar again. I just wasn't really sure whether `.dev_groups` in class behaves the same as with what Steffen did. From the documentation it does though. > > Thanks, > > Bart. > > > Subject: [PATCH] scsi: core: Remove Scsi_Host.shost_dev_attr_groups > > Suggested-by: Benjamin Block <bblock@linux.ibm.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/hosts.c | 15 +++------------ > drivers/scsi/scsi_priv.h | 2 +- > drivers/scsi/scsi_sysfs.c | 7 ++++++- > include/scsi/scsi_host.h | 6 ------ > 4 files changed, 10 insertions(+), 20 deletions(-) > > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c > index 17aef936bc90..f88e7ed77dbb 100644 > --- a/drivers/scsi/hosts.c > +++ b/drivers/scsi/hosts.c > @@ -61,6 +61,7 @@ static void scsi_host_cls_release(struct device *dev) > static struct class shost_class = { > .name = "scsi_host", > .dev_release = scsi_host_cls_release, > + .dev_groups = scsi_shost_groups, > }; > > /** > @@ -376,7 +377,7 @@ static struct device_type scsi_host_type = { > struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) > { > struct Scsi_Host *shost; > - int index, i, j = 0; > + int index; > > shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); > if (!shost) > @@ -481,17 +482,7 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize) > shost->shost_dev.parent = &shost->shost_gendev; > shost->shost_dev.class = &shost_class; > dev_set_name(&shost->shost_dev, "host%d", shost->host_no); > - shost->shost_dev.groups = shost->shost_dev_attr_groups; > - shost->shost_dev_attr_groups[j++] = &scsi_shost_attr_group; > - if (sht->shost_groups) { > - for (i = 0; sht->shost_groups[i] && > - j < ARRAY_SIZE(shost->shost_dev_attr_groups); > - i++, j++) { > - shost->shost_dev_attr_groups[j] = > - sht->shost_groups[i]; > - } > - } > - WARN_ON_ONCE(j >= ARRAY_SIZE(shost->shost_dev_attr_groups)); > + shost->shost_dev.groups = sht->shost_groups; > > shost->ehandler = kthread_run(scsi_error_handler, shost, > "scsi_eh_%d", shost->host_no); > diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h > index a278fc8948f4..0f5743f4769b 100644 > --- a/drivers/scsi/scsi_priv.h > +++ b/drivers/scsi/scsi_priv.h > @@ -144,7 +144,7 @@ extern struct scsi_transport_template blank_transport_template; > extern void __scsi_remove_device(struct scsi_device *); > > extern struct bus_type scsi_bus_type; > -extern const struct attribute_group scsi_shost_attr_group; > +extern const struct attribute_group *scsi_shost_groups[]; > > /* scsi_netlink.c */ > #ifdef CONFIG_SCSI_NETLINK > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index c26f0e29e8cd..f360154b5241 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -424,10 +424,15 @@ static struct attribute *scsi_sysfs_shost_attrs[] = { > NULL > }; > > -const struct attribute_group scsi_shost_attr_group = { > +static const struct attribute_group scsi_shost_attr_group = { > .attrs = scsi_sysfs_shost_attrs, > }; > > +const struct attribute_group *scsi_shost_groups[] = { > + &scsi_shost_attr_group, > + NULL > +}; > + > static void scsi_device_cls_release(struct device *class_dev) > { > struct scsi_device *sdev; > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index ae715959f886..97cdad14de56 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -690,12 +690,6 @@ struct Scsi_Host { > > /* ldm bits */ > struct device shost_gendev, shost_dev; > - /* > - * The array size 3 provides space for one attribute group defined by > - * the SCSI core, one attribute group defined by the SCSI LLD and one > - * terminating NULL pointer. > - */ > - const struct attribute_group *shost_dev_attr_groups[3]; > > /* > * Points to the transport data (if any) which is allocated Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
On Tue, Oct 26, 2021 at 03:42:40AM +0200, Steffen Maier wrote: > v4.17 commit 86b87cde0b55 ("scsi: core: host template attribute groups") > introduced explicit sysfs_create_groups() in scsi_sysfs_add_sdev() > and sysfs_remove_groups() in __scsi_remove_device(), both for sdev_gendev, > based on a new field const struct attribute_group **sdev_groups > of struct scsi_host_template. > > Commit 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") > removed above explicit (de)registration of scsi_device attribute groups. > It also converted all scsi_device attributes and attribute_groups to > end up in a new field const struct attribute_group *gendev_attr_groups[6] > of struct scsi_device. However, that new field was not used anywhere. > > Surprisingly, this only caused missing LLDD specific scsi_device sysfs > attributes. Whereas, scsi core attributes from scsi_sdev_attr_groups > did continue to exist because of scsi_dev_type.groups. > > We separate scsi core attibutes from LLDD specific attributes. > Hence, we keep the initializing assignment scsi_dev_type = > { .groups = scsi_sdev_attr_groups, } as this takes care of core > attributes. Without the separation, it would cause attribute double > registration due to scsi_dev_type.groups and sdev_gendev.groups. > > Julian suggested to assign the sdev_groups pointer of the > scsi_host_template directly to the groups pointer of sdev_gendev. > This way we can delete the container scsi_device.gendev_attr_groups > and the loop copying each entry from hostt->sdev_groups to > sdev->gendev_attr_groups. > > Alternative approaches ruled out: > Assigning gendev_attr_groups to sdev_dev has no visible effect. > Assigning sdev->gendev_attr_groups to scsi_dev_type.groups > caused scsi_device of all scsi host types to get LLDD specific > attributes of the LLDD for which the last sdev alloc happened to occur, > as that overwrote scsi_dev_type.groups, > e.g. scsi_debug had zfcp-specific scsi_device attributes. > > Signed-off-by: Steffen Maier <maier@linux.ibm.com> > Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") > Suggested-by: Julian Wiedmann <jwi@linux.ibm.com> > --- > > Notes: > Changes in v3: > * integrated Julian's feedback of dropping detour through > gendev_attr_groups > > Changes in v2: > * integrated Bart's feedback of updating the comment for > the gendev_attr_groups declaration to match the code change > * in that spirit also adapted the vector size of that field > * eliminated the now unnecessary second loop counter 'j' > > drivers/scsi/scsi_sysfs.c | 11 +---------- > include/scsi/scsi_device.h | 6 ------ > 2 files changed, 1 insertion(+), 16 deletions(-) > Looks good to me. Reviewed-by: Benjamin Block <bblock@linux.ibm.com>
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index c26f0e29e8cd..fa064bf9a55c 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1571,7 +1571,6 @@ static struct device_type scsi_dev_type = { void scsi_sysfs_device_initialize(struct scsi_device *sdev) { - int i, j = 0; unsigned long flags; struct Scsi_Host *shost = sdev->host; struct scsi_host_template *hostt = shost->hostt; @@ -1583,15 +1582,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) scsi_enable_async_suspend(&sdev->sdev_gendev); dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%llu", sdev->host->host_no, sdev->channel, sdev->id, sdev->lun); - sdev->gendev_attr_groups[j++] = &scsi_sdev_attr_group; - if (hostt->sdev_groups) { - for (i = 0; hostt->sdev_groups[i] && - j < ARRAY_SIZE(sdev->gendev_attr_groups); - i++, j++) { - sdev->gendev_attr_groups[j] = hostt->sdev_groups[i]; - } - } - WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); + sdev->sdev_gendev.groups = hostt->sdev_groups; device_initialize(&sdev->sdev_dev); sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index b1e9b3bd3a60..b97e142a7ca9 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -225,12 +225,6 @@ struct scsi_device { struct device sdev_gendev, sdev_dev; - /* - * The array size 6 provides space for one attribute group for the - * SCSI core, four attribute groups defined by SCSI LLDs and one - * terminating NULL pointer. - */ - const struct attribute_group *gendev_attr_groups[6]; struct execute_work ew; /* used to get process context on put */ struct work_struct requeue_work;
v4.17 commit 86b87cde0b55 ("scsi: core: host template attribute groups") introduced explicit sysfs_create_groups() in scsi_sysfs_add_sdev() and sysfs_remove_groups() in __scsi_remove_device(), both for sdev_gendev, based on a new field const struct attribute_group **sdev_groups of struct scsi_host_template. Commit 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") removed above explicit (de)registration of scsi_device attribute groups. It also converted all scsi_device attributes and attribute_groups to end up in a new field const struct attribute_group *gendev_attr_groups[6] of struct scsi_device. However, that new field was not used anywhere. Surprisingly, this only caused missing LLDD specific scsi_device sysfs attributes. Whereas, scsi core attributes from scsi_sdev_attr_groups did continue to exist because of scsi_dev_type.groups. We separate scsi core attibutes from LLDD specific attributes. Hence, we keep the initializing assignment scsi_dev_type = { .groups = scsi_sdev_attr_groups, } as this takes care of core attributes. Without the separation, it would cause attribute double registration due to scsi_dev_type.groups and sdev_gendev.groups. Julian suggested to assign the sdev_groups pointer of the scsi_host_template directly to the groups pointer of sdev_gendev. This way we can delete the container scsi_device.gendev_attr_groups and the loop copying each entry from hostt->sdev_groups to sdev->gendev_attr_groups. Alternative approaches ruled out: Assigning gendev_attr_groups to sdev_dev has no visible effect. Assigning sdev->gendev_attr_groups to scsi_dev_type.groups caused scsi_device of all scsi host types to get LLDD specific attributes of the LLDD for which the last sdev alloc happened to occur, as that overwrote scsi_dev_type.groups, e.g. scsi_debug had zfcp-specific scsi_device attributes. Signed-off-by: Steffen Maier <maier@linux.ibm.com> Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") Suggested-by: Julian Wiedmann <jwi@linux.ibm.com> --- Notes: Changes in v3: * integrated Julian's feedback of dropping detour through gendev_attr_groups Changes in v2: * integrated Bart's feedback of updating the comment for the gendev_attr_groups declaration to match the code change * in that spirit also adapted the vector size of that field * eliminated the now unnecessary second loop counter 'j' drivers/scsi/scsi_sysfs.c | 11 +---------- include/scsi/scsi_device.h | 6 ------ 2 files changed, 1 insertion(+), 16 deletions(-)