| Message ID | 20211024221620.760160-1-maier@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] scsi: core: Fix early registration of sysfs attributes for scsi_device | expand |
On 25.10.21 00:16, 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. > ... > Signed-off-by: Steffen Maier <maier@linux.ibm.com> > Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") > --- > > 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 | 12 ++++++------ > include/scsi/scsi_device.h | 7 +++---- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index c26f0e29e8cd..d73e84e1cb37 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1571,7 +1571,7 @@ static struct device_type scsi_dev_type = { > > void scsi_sysfs_device_initialize(struct scsi_device *sdev) > { > - int i, j = 0; > + int i = 0; > unsigned long flags; > struct Scsi_Host *shost = sdev->host; > struct scsi_host_template *hostt = shost->hostt; > @@ -1583,15 +1583,15 @@ 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; > + sdev->sdev_gendev.groups = sdev->gendev_attr_groups; > 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]; > + i < ARRAY_SIZE(sdev->gendev_attr_groups); > + i++) { > + sdev->gendev_attr_groups[i] = hostt->sdev_groups[i]; > } > } > - WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); > + WARN_ON_ONCE(i >= ARRAY_SIZE(sdev->gendev_attr_groups)); > Can't we simply assign the hostt->sdev_groups now, without the additional indirection? 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..b6f0d031217e 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -226,11 +226,10 @@ 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. > + * The array size 5 provides space for four attribute groups > + * defined by SCSI LLDs and one terminating NULL pointer. > */ > - const struct attribute_group *gendev_attr_groups[6]; > + const struct attribute_group *gendev_attr_groups[5]; > > struct execute_work ew; /* used to get process context on put */ > struct work_struct requeue_work; >
On 10/25/21 2:23 AM, Julian Wiedmann wrote: > On 25.10.21 00:16, Steffen Maier wrote: >> void scsi_sysfs_device_initialize(struct scsi_device *sdev) >> { >> - int i, j = 0; >> + int i = 0; >> unsigned long flags; >> struct Scsi_Host *shost = sdev->host; >> struct scsi_host_template *hostt = shost->hostt; >> @@ -1583,15 +1583,15 @@ 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; >> + sdev->sdev_gendev.groups = sdev->gendev_attr_groups; >> 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]; >> + i < ARRAY_SIZE(sdev->gendev_attr_groups); >> + i++) { >> + sdev->gendev_attr_groups[i] = hostt->sdev_groups[i]; >> } >> } >> - WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); >> + WARN_ON_ONCE(i >= ARRAY_SIZE(sdev->gendev_attr_groups)); >> > > Can't we simply assign the hostt->sdev_groups now, without the additional > indirection? > > sdev->sdev_gendev.groups = hostt->sdev_groups; Hi Steffen, Please let me know if you wouldn't have the time to integrate the approach mentioned above in a new version of your patch. Thanks, Bart.
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index c26f0e29e8cd..d73e84e1cb37 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1571,7 +1571,7 @@ static struct device_type scsi_dev_type = { void scsi_sysfs_device_initialize(struct scsi_device *sdev) { - int i, j = 0; + int i = 0; unsigned long flags; struct Scsi_Host *shost = sdev->host; struct scsi_host_template *hostt = shost->hostt; @@ -1583,15 +1583,15 @@ 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; + sdev->sdev_gendev.groups = sdev->gendev_attr_groups; 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]; + i < ARRAY_SIZE(sdev->gendev_attr_groups); + i++) { + sdev->gendev_attr_groups[i] = hostt->sdev_groups[i]; } } - WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); + WARN_ON_ONCE(i >= ARRAY_SIZE(sdev->gendev_attr_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..b6f0d031217e 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -226,11 +226,10 @@ 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. + * The array size 5 provides space for four attribute groups + * defined by SCSI LLDs and one terminating NULL pointer. */ - const struct attribute_group *gendev_attr_groups[6]; + const struct attribute_group *gendev_attr_groups[5]; 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. Fix it by assigning the pointer of that new field to the groups field of sdev_gendev so the driver core gets our LLDD groups. Just like scsi_host_alloc() was changed by the same commit, although scsi_host_alloc() already had assigned something to shost_dev.groups so the necessary change was more obvious there. 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. Since we now no longer have an index offset between left and right hand side of the assignment in the loop, one counter variable suffices. 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") --- 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 | 12 ++++++------ include/scsi/scsi_device.h | 7 +++---- 2 files changed, 9 insertions(+), 10 deletions(-)