Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]

Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]

[Steffen Maier]

Hi Bart, hi Martin,

since Friday 2021-10-22 our CI reports errors with linux-next. It complains 
about missing zfcp-lun resources (although that's a follow-on error). Machines 
that have their root-fs on zfcp-attached SCSI disk(s) are stuck in boot. 
Looking at user visible effects, I see zfcp-specific sysfs attributes of 
scsi_device missing:

$ lszfcp -D
/usr/sbin/lszfcp: line 390: 
/sys/bus/ccw/drivers/zfcp/0.0.1780/host0/rport-0:0-0/target0:0:0/0:0:0:1089224725//hba_id: 
No such file or directory
/usr/sbin/lszfcp: line 391: 
/sys/bus/ccw/drivers/zfcp/0.0.1780/host0/rport-0:0-0/target0:0:0/0:0:0:1089224725//wwpn: 
No such file or directory
/usr/sbin/lszfcp: line 392: 
/sys/bus/ccw/drivers/zfcp/0.0.1780/host0/rport-0:0-0/target0:0:0/0:0:0:1089224725//fcp_lun: 
No such file or directory

That made me think of this patch series. It also happened so that Martin 
applied the series to 5.16/scsi-queue on 2021-10-21. Linux-next merged it on 
2021-10-22:

Merging scsi-mkp/for-next (83c3a7beaef7 scsi: lpfc: Update lpfc version to 
14.0.0.3)
$ git merge -m Merge branch 'for-next' of 
git://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git scsi-mkp/for-next
...
+ drivers/s390/scsi/zfcp_ext.h                    |   4 +-
+ drivers/s390/scsi/zfcp_fsf.c                    |   2 +-
+ drivers/s390/scsi/zfcp_scsi.c                   |   8 +-
+ drivers/s390/scsi/zfcp_sysfs.c                  |  52 ++--
[https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20211022&id=cf6c9d12750cf6f3f6aeffcd0bdb36b1ac993f44]

So this seems to match with the occurrence of problems for us.

I wonder if any of the other LLDDs see similar problems. I left those LLDD 
patches in the list below, that also were migrated from sdev_attrs to sdev_groups.

Guess it would be good to fix this before the v5.16 merge window opens 
(2021-11-08 after predicted v5.15 release? [http://phb-crystal-ball.org/]) so 
the error does not land in Linus' tree (which our CI also tests).

Not sure if I'll find time to dig deeper.

On 10/21/21 05:42, Martin K. Petersen wrote:
> On Tue, 12 Oct 2021 16:35:12 -0700, Bart Van Assche wrote:
> 
>> For certain user space software, e.g. udev, it is important that sysfs
>> attributes are registered before the KOBJ_ADD uevent is emitted. Hence
>> this patch series that removes the device_create_file() and
>> sysfs_create_groups() calls from the SCSI core. Please consider this
>> patch series for kernel v5.16.
>>
>> Thanks,
>>
>> [...]
> 
> Applied to 5.16/scsi-queue, thanks!
> 
> [01/46] scsi: core: Register sysfs attributes earlier
>          https://git.kernel.org/mkp/scsi/c/92c4b58b15c5
> [02/46] ata: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/c3f69c7f629f
> [03/46] firewire: sbp2: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/5e88e67b6f3b

> [06/46] scsi: zfcp: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/d8d7cf3f7d07

> [10/46] scsi: 53c700: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/90cb6538b5da
> [11/46] scsi: aacraid: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/bd16d71185c8

> [18/46] scsi: cxlflash: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/780c678912fb

> [21/46] scsi: hpsa: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/4cd16323b523

> [25/46] scsi: ipr: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/47d1e6ae0e1e

> [28/46] scsi: megaraid: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/09723bb252ca
> [29/46] scsi: mpt3sas: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/1bb3ca27d2ca

> [31/46] scsi: myrb: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/582c0360db90
> [32/46] scsi: myrs: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/087c3ace6337

> [42/46] scsi: smartpqi: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/64fc9015fbeb

> [45/46] scsi: usb: Switch to attribute groups
>          https://git.kernel.org/mkp/scsi/c/01e570febaaa
> [46/46] scsi: core: Remove two host template members that are no longer used
>          https://git.kernel.org/mkp/scsi/c/a47c6b713e89



-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z and LinuxONE

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]

[Steffen Maier]

On 10/23/21 22:54, Steffen Maier wrote:
> Hi Bart, hi Martin,
> 
> since Friday 2021-10-22 our CI reports errors with linux-next. It complains 
> about missing zfcp-lun resources (although that's a follow-on error). Machines 
> that have their root-fs on zfcp-attached SCSI disk(s) are stuck in boot. 
> Looking at user visible effects, I see zfcp-specific sysfs attributes of 
> scsi_device missing:
> 
> $ lszfcp -D
> /usr/sbin/lszfcp: line 390: 
> /sys/bus/ccw/drivers/zfcp/0.0.1780/host0/rport-0:0-0/target0:0:0/0:0:0:1089224725//hba_id: 
> No such file or directory

> That made me think of this patch series. It also happened so that Martin 
> applied the series to 5.16/scsi-queue on 2021-10-21. Linux-next merged it on 
> 2021-10-22:

> So this seems to match with the occurrence of problems for us.
> 
> I wonder if any of the other LLDDs see similar problems. I left those LLDD 
> patches in the list below, that also were migrated from sdev_attrs to sdev_groups.
> 
> Guess it would be good to fix this before the v5.16 merge window opens 
> (2021-11-08 after predicted v5.15 release? [http://phb-crystal-ball.org/]) so 
> the error does not land in Linus' tree (which our CI also tests).
> 
> Not sure if I'll find time to dig deeper.

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 is not used anywhere.

I tried to fix it by assigning the pointer of that new field to the groups
field of sdev_gendev so the driver core gets our groups on devide_add().
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.

However, that gave me:

> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: scsi scan: INQUIRY pass 1 length 36
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: scsi scan: INQUIRY successful with code 0x0
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: scsi scan: INQUIRY pass 2 length 164
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: scsi scan: INQUIRY successful with code 0x0
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: Direct-Access     IBM      2107900          2.19 PQ: 0 ANSI: 5
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: alua: supports implicit TPGS
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: alua: device naa.6005076309ffd43000000000000015fb port group 0 rel port 103
> Oct 24 02:06:20 t3545002 kernel: sysfs: cannot create duplicate filename '/devices/css0/0.0.0019/0.0.1880/host1/rport-1:0-2/target1:0:2/1:0:2:1090207765/device_blocked'
> Oct 24 02:06:20 t3545002 kernel: CPU: 1 PID: 1530 Comm: chzdev Not tainted 5.15.0-rc1sdevattr+ #29
> Oct 24 02:06:20 t3545002 kernel: Hardware name: IBM 8561 T01 703 (z/VM 7.2.0)
> Oct 24 02:06:20 t3545002 kernel: Call Trace:
> Oct 24 02:06:20 t3545002 kernel: [<00000000701c923e>] dump_stack_lvl+0x8e/0xc8 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f991408>] sysfs_warn_dup+0x78/0x88 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f990fb0>] sysfs_add_file_mode_ns+0x1c8/0x1e8 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f991e3c>] create_files+0xb4/0x230 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f992068>] internal_create_group+0xb0/0x1e8 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f9928b0>] internal_create_groups.part.0+0x60/0xc8 
> Oct 24 02:06:20 t3545002 kernel: [<000000006fe807ca>] device_add_attrs+0x72/0x1d0 
> Oct 24 02:06:20 t3545002 kernel: [<000000006fe85168>] device_add+0x360/0x690 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff801429b0>] scsi_sysfs_add_sdev+0x60/0x1a0 [scsi_mod] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff8013d870>] scsi_add_lun+0x3b0/0x5d0 [scsi_mod] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff8013e62c>] scsi_probe_and_add_lun+0x184/0x4a0 [scsi_mod] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff8013f4ec>] __scsi_scan_target+0x9c/0x240 [scsi_mod] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff8013f76a>] scsi_scan_target+0xda/0xf8 [scsi_mod] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff802d2a9c>] zfcp_unit_scsi_scan+0x6c/0x78 [zfcp] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff802d2e32>] zfcp_unit_add+0x1ea/0x220 [zfcp] 
> Oct 24 02:06:20 t3545002 kernel: [<000003ff802d0f3c>] zfcp_sysfs_unit_add_store+0x4c/0x70 [zfcp] 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f98f946>] kernfs_fop_write_iter+0x13e/0x1e0 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f899ff8>] new_sync_write+0x100/0x190 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f89ced0>] vfs_write+0x230/0x2e0 
> Oct 24 02:06:20 t3545002 kernel: [<000000006f89d1ec>] ksys_write+0x6c/0xf8 
> Oct 24 02:06:20 t3545002 kernel: [<00000000701cc7ba>] __do_syscall+0x1c2/0x1f0 
> Oct 24 02:06:20 t3545002 kernel: [<00000000701df168>] system_call+0x78/0xa0 
> Oct 24 02:06:20 t3545002 kernel: 4 locks held by chzdev/1530:
> Oct 24 02:06:20 t3545002 kernel: #0: 0000000003711498 (sb_writers#3){.+.+}-{0:0}, at: ksys_write+0x6c/0xf8
> Oct 24 02:06:20 t3545002 kernel: #1: 000000000e77fa90 (&of->mutex){+.+.}-{3:3}, at: kernfs_fop_write_iter+0x102/0x1e0
> Oct 24 02:06:20 t3545002 kernel: #2: 0000000035754e10 (kn->active#83){++++}-{0:0}, at: kernfs_fop_write_iter+0x10e/0x1e0
> Oct 24 02:06:20 t3545002 kernel: #3: 0000000031bf90f0 (&shost->scan_mutex){+.+.}-{3:3}, at: scsi_scan_target+0x90/0xf8 [scsi_mod]
> Oct 24 02:06:20 t3545002 kernel: scsi 1:0:2:1090207765: failed to add device: -17

So I suspected that sdev_gendev is not a good idea and tried with its child 
sdev_dev instead. But now I'm back at square one with the attributes missing.

Where can I read about sdev_gendev vs. sdev_dev in a hopefully easy to 
understand way?

And how do we get to pass our scsi_device.gendev_attr_groups to the driver core 
to appear in sysfs?

> On 10/21/21 05:42, Martin K. Petersen wrote:
>> On Tue, 12 Oct 2021 16:35:12 -0700, Bart Van Assche wrote:
>>
>>> For certain user space software, e.g. udev, it is important that sysfs
>>> attributes are registered before the KOBJ_ADD uevent is emitted. Hence
>>> this patch series that removes the device_create_file() and
>>> sysfs_create_groups() calls from the SCSI core. Please consider this
>>> patch series for kernel v5.16.
>>>
>>> Thanks,
>>>
>>> [...]
>>
>> Applied to 5.16/scsi-queue, thanks!
>>
>> [01/46] scsi: core: Register sysfs attributes earlier
>>          https://git.kernel.org/mkp/scsi/c/92c4b58b15c5
>> [02/46] ata: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/c3f69c7f629f
>> [03/46] firewire: sbp2: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/5e88e67b6f3b
> 
>> [06/46] scsi: zfcp: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/d8d7cf3f7d07
> 
>> [10/46] scsi: 53c700: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/90cb6538b5da
>> [11/46] scsi: aacraid: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/bd16d71185c8
> 
>> [18/46] scsi: cxlflash: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/780c678912fb
> 
>> [21/46] scsi: hpsa: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/4cd16323b523
> 
>> [25/46] scsi: ipr: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/47d1e6ae0e1e
> 
>> [28/46] scsi: megaraid: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/09723bb252ca
>> [29/46] scsi: mpt3sas: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/1bb3ca27d2ca
> 
>> [31/46] scsi: myrb: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/582c0360db90
>> [32/46] scsi: myrs: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/087c3ace6337
> 
>> [42/46] scsi: smartpqi: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/64fc9015fbeb
> 
>> [45/46] scsi: usb: Switch to attribute groups
>>          https://git.kernel.org/mkp/scsi/c/01e570febaaa
>> [46/46] scsi: core: Remove two host template members that are no longer used
>>          https://git.kernel.org/mkp/scsi/c/a47c6b713e89
-- 
Mit freundlichen Gruessen / Kind regards
Steffen Maier

Linux on IBM Z and LinuxONE

https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]

[Bart Van Assche]

On 10/23/21 17:39, Steffen Maier wrote:
> I tried to fix it by assigning the pointer of that new field to the groups
> field of sdev_gendev so the driver core gets our groups on devide_add().
> 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.

Thanks for having reported this early. We probably need something like this
(entirely untested):

--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1583,7 +1583,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;
+	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);

Bart.

[PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Steffen Maier]

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.

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")
---
 drivers/scsi/scsi_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index c26f0e29e8cd..a3e37d3728df 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1583,7 +1583,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;
+	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);
-- 
2.25.1

Re: [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Bart Van Assche]

On 10/24/21 04:18, Steffen Maier wrote:
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index c26f0e29e8cd..a3e37d3728df 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -1583,7 +1583,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;
> +	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);

How about also updating the comment above the gendev_attr_groups
declaration since the above change makes that comment incorrect?

Thanks,

Bart.

[PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Steffen Maier]

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(-)

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;
-- 
2.25.1

Re: [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Julian Wiedmann]

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;
> 

Re: [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Bart Van Assche]

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.

[PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Steffen Maier]

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(-)

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;
-- 
2.25.1

Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Benjamin Block]

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>

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Bart Van Assche]

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

Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Benjamin Block]

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>

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294

Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Bart Van Assche]

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>

Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device

[Benjamin Block]

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>

-- 
Best Regards, Benjamin Block  / Linux on IBM Z Kernel Development / IBM Systems
IBM Deutschland Research & Development GmbH    /    https://www.ibm.com/privacy
Vorsitz. AufsR.: Gregor Pillen         /        Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294