| Message ID | 20211111084551.446548-1-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | scsi: fix scsi device attributes registration | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 11/11/21 09:45, Damien Le Moal wrote: > Since the sdev_gendev device of a scsi device defines its attributes > using scsi_sdev_attr_groups as the groups field value of its device > type, the execution of device_add() in scsi_sysfs_add_sdev() register > with sysfs only the attributes defined using scsi_sdev_attr_groups. As > a results, the attributes defined by an LLD using the scsi host > sdev_groups attribute groups are never registered with sysfs and not > visible to the users. > > Fix this problem by removing scsi_sdev_attr_groups and manually setting > the groups field of a scsi device sdev_gendev to point to the scsi > device gendev_attr_groups. As the first entry of this array of > attribute groups is scsi_sdev_attr_group, using gendev_attr_groups as > the gendev groups result in all defined attributes to be created in > sysfs when device_add() is called. > > Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") > cc: Bart Van Assche <bvanassche@acm.org> > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/scsi/scsi_sysfs.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c > index d3d362289ecc..92c92853f516 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1301,11 +1301,6 @@ static struct attribute_group scsi_sdev_attr_group = { > .is_bin_visible = scsi_sdev_bin_attr_is_visible, > }; > > -static const struct attribute_group *scsi_sdev_attr_groups[] = { > - &scsi_sdev_attr_group, > - NULL > -}; > - > static int scsi_target_add(struct scsi_target *starget) > { > int error; > @@ -1575,7 +1570,6 @@ int scsi_sysfs_add_host(struct Scsi_Host *shost) > static struct device_type scsi_dev_type = { > .name = "scsi_device", > .release = scsi_device_dev_release, > - .groups = scsi_sdev_attr_groups, > }; > > void scsi_sysfs_device_initialize(struct scsi_device *sdev) > @@ -1601,6 +1595,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) > } > } > WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); > + sdev->sdev_gendev.groups = sdev->gendev_attr_groups; > > device_initialize(&sdev->sdev_dev); > sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev); > Damien, which kernel were you using? Isn't this fixed by?: next: https://lore.kernel.org/linux-scsi/163478764102.7011.9375895285870786953.b4-ty@oracle.com/t/#mab0eeb4a8d8db95c3ace0013bfef775736e124cb ("scsi: core: Fix early registration of sysfs attributes for scsi_device") https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=5.16/scsi-staging&id=3a71f0f7a51259b3cb95d79cac1e19dcc5e89ce9 https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=5.16/scsi-queue&id=3a71f0f7a51259b3cb95d79cac1e19dcc5e89ce9 https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=for-next&id=3a71f0f7a51259b3cb95d79cac1e19dcc5e89ce9 => https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?h=next-20211028&id=503f375baa99edff894eb1a534d2ac0b4f799573 soon in vanilla: https://lore.kernel.org/linux-scsi/163612851260.17201.4106345384610850520.pr-tracker-bot@kernel.org/t/#md79869a3966ccceb30eef658b85743e49cf31dc1
On 2021/11/11 19:35, Steffen Maier wrote: > On 11/11/21 09:45, Damien Le Moal wrote: >> Since the sdev_gendev device of a scsi device defines its attributes >> using scsi_sdev_attr_groups as the groups field value of its device >> type, the execution of device_add() in scsi_sysfs_add_sdev() register >> with sysfs only the attributes defined using scsi_sdev_attr_groups. As >> a results, the attributes defined by an LLD using the scsi host >> sdev_groups attribute groups are never registered with sysfs and not >> visible to the users. >> >> Fix this problem by removing scsi_sdev_attr_groups and manually setting >> the groups field of a scsi device sdev_gendev to point to the scsi >> device gendev_attr_groups. As the first entry of this array of >> attribute groups is scsi_sdev_attr_group, using gendev_attr_groups as >> the gendev groups result in all defined attributes to be created in >> sysfs when device_add() is called. >> >> Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") >> cc: Bart Van Assche <bvanassche@acm.org> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/scsi/scsi_sysfs.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c >> index d3d362289ecc..92c92853f516 100644 >> --- a/drivers/scsi/scsi_sysfs.c >> +++ b/drivers/scsi/scsi_sysfs.c >> @@ -1301,11 +1301,6 @@ static struct attribute_group scsi_sdev_attr_group = { >> .is_bin_visible = scsi_sdev_bin_attr_is_visible, >> }; >> >> -static const struct attribute_group *scsi_sdev_attr_groups[] = { >> - &scsi_sdev_attr_group, >> - NULL >> -}; >> - >> static int scsi_target_add(struct scsi_target *starget) >> { >> int error; >> @@ -1575,7 +1570,6 @@ int scsi_sysfs_add_host(struct Scsi_Host *shost) >> static struct device_type scsi_dev_type = { >> .name = "scsi_device", >> .release = scsi_device_dev_release, >> - .groups = scsi_sdev_attr_groups, >> }; >> >> void scsi_sysfs_device_initialize(struct scsi_device *sdev) >> @@ -1601,6 +1595,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) >> } >> } >> WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); >> + sdev->sdev_gendev.groups = sdev->gendev_attr_groups; >> >> device_initialize(&sdev->sdev_dev); >> sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev); >> > > Damien, which kernel were you using? > > Isn't this fixed by?: > > next: > > https://lore.kernel.org/linux-scsi/163478764102.7011.9375895285870786953.b4-ty@oracle.com/t/#mab0eeb4a8d8db95c3ace0013bfef775736e124cb > ("scsi: core: Fix early registration of sysfs attributes for scsi_device") > https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=5.16/scsi-staging&id=3a71f0f7a51259b3cb95d79cac1e19dcc5e89ce9 > https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=5.16/scsi-queue&id=3a71f0f7a51259b3cb95d79cac1e19dcc5e89ce9 > https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=for-next&id=3a71f0f7a51259b3cb95d79cac1e19dcc5e89ce9 > => > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?h=next-20211028&id=503f375baa99edff894eb1a534d2ac0b4f799573 > > soon in vanilla: > > https://lore.kernel.org/linux-scsi/163612851260.17201.4106345384610850520.pr-tracker-bot@kernel.org/t/#md79869a3966ccceb30eef658b85743e49cf31dc1 I discovered the problem while testing libata tree which is currently based on Linus master. So I completely missed this fix, which is even simpler than what I sent :) I should have checked Martin's tree first ! Thanks for pointing this out. I tested and everything is fine. Martin, Bart, My apologies for the noise :)
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index d3d362289ecc..92c92853f516 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1301,11 +1301,6 @@ static struct attribute_group scsi_sdev_attr_group = { .is_bin_visible = scsi_sdev_bin_attr_is_visible, }; -static const struct attribute_group *scsi_sdev_attr_groups[] = { - &scsi_sdev_attr_group, - NULL -}; - static int scsi_target_add(struct scsi_target *starget) { int error; @@ -1575,7 +1570,6 @@ int scsi_sysfs_add_host(struct Scsi_Host *shost) static struct device_type scsi_dev_type = { .name = "scsi_device", .release = scsi_device_dev_release, - .groups = scsi_sdev_attr_groups, }; void scsi_sysfs_device_initialize(struct scsi_device *sdev) @@ -1601,6 +1595,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev) } } WARN_ON_ONCE(j >= ARRAY_SIZE(sdev->gendev_attr_groups)); + sdev->sdev_gendev.groups = sdev->gendev_attr_groups; device_initialize(&sdev->sdev_dev); sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
Since the sdev_gendev device of a scsi device defines its attributes using scsi_sdev_attr_groups as the groups field value of its device type, the execution of device_add() in scsi_sysfs_add_sdev() register with sysfs only the attributes defined using scsi_sdev_attr_groups. As a results, the attributes defined by an LLD using the scsi host sdev_groups attribute groups are never registered with sysfs and not visible to the users. Fix this problem by removing scsi_sdev_attr_groups and manually setting the groups field of a scsi device sdev_gendev to point to the scsi device gendev_attr_groups. As the first entry of this array of attribute groups is scsi_sdev_attr_group, using gendev_attr_groups as the gendev groups result in all defined attributes to be created in sysfs when device_add() is called. Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier") cc: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/scsi_sysfs.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)