* Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]
[not found] ` <163478764102.7011.9375895285870786953.b4-ty@oracle.com>
@ 2021-10-23 20:54 ` Steffen Maier
2021-10-24 0:39 ` Steffen Maier
0 siblings, 1 reply; 14+ messages in thread
From: Steffen Maier @ 2021-10-23 20:54 UTC (permalink / raw)
To: Martin K. Petersen, Bart Van Assche
Cc: linux-scsi, Linux-Next Mailing List, linux-s390, Benjamin Block
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]
2021-10-23 20:54 ` Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier] Steffen Maier
@ 2021-10-24 0:39 ` Steffen Maier
2021-10-24 2:55 ` Bart Van Assche
2021-10-24 11:18 ` [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device Steffen Maier
0 siblings, 2 replies; 14+ messages in thread
From: Steffen Maier @ 2021-10-24 0:39 UTC (permalink / raw)
To: Martin K. Petersen, Bart Van Assche
Cc: linux-scsi, Linux-Next Mailing List, linux-s390, Benjamin Block
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier]
2021-10-24 0:39 ` Steffen Maier
@ 2021-10-24 2:55 ` Bart Van Assche
2021-10-24 11:18 ` [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device Steffen Maier
1 sibling, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2021-10-24 2:55 UTC (permalink / raw)
To: Steffen Maier, Martin K. Petersen
Cc: linux-scsi, Linux-Next Mailing List, linux-s390, Benjamin Block
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-24 0:39 ` Steffen Maier
2021-10-24 2:55 ` Bart Van Assche
@ 2021-10-24 11:18 ` Steffen Maier
2021-10-24 21:25 ` Bart Van Assche
1 sibling, 1 reply; 14+ messages in thread
From: Steffen Maier @ 2021-10-24 11:18 UTC (permalink / raw)
To: bvanassche, martin.petersen, jejb, linux-scsi
Cc: gregkh, maier, bblock, linux-next, linux-s390
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-24 11:18 ` [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device Steffen Maier
@ 2021-10-24 21:25 ` Bart Van Assche
2021-10-24 22:16 ` [PATCH v2] " Steffen Maier
0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2021-10-24 21:25 UTC (permalink / raw)
To: Steffen Maier, martin.petersen, jejb, linux-scsi
Cc: gregkh, bblock, linux-next, linux-s390
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-24 21:25 ` Bart Van Assche
@ 2021-10-24 22:16 ` Steffen Maier
2021-10-25 9:23 ` Julian Wiedmann
0 siblings, 1 reply; 14+ messages in thread
From: Steffen Maier @ 2021-10-24 22:16 UTC (permalink / raw)
To: bvanassche, martin.petersen, jejb, linux-scsi
Cc: gregkh, maier, bblock, linux-next, linux-s390
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-24 22:16 ` [PATCH v2] " Steffen Maier
@ 2021-10-25 9:23 ` Julian Wiedmann
2021-10-25 16:00 ` Bart Van Assche
2021-10-26 1:42 ` [PATCH v3] " Steffen Maier
0 siblings, 2 replies; 14+ messages in thread
From: Julian Wiedmann @ 2021-10-25 9:23 UTC (permalink / raw)
To: Steffen Maier, bvanassche, martin.petersen, jejb, linux-scsi
Cc: gregkh, bblock, linux-next, linux-s390
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;
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-25 9:23 ` Julian Wiedmann
@ 2021-10-25 16:00 ` Bart Van Assche
2021-10-26 1:42 ` [PATCH v3] " Steffen Maier
1 sibling, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2021-10-25 16:00 UTC (permalink / raw)
To: Julian Wiedmann, Steffen Maier, martin.petersen, jejb, linux-scsi
Cc: gregkh, bblock, linux-next, linux-s390
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-25 9:23 ` Julian Wiedmann
2021-10-25 16:00 ` Bart Van Assche
@ 2021-10-26 1:42 ` Steffen Maier
2021-10-26 10:00 ` Benjamin Block
` (2 more replies)
1 sibling, 3 replies; 14+ messages in thread
From: Steffen Maier @ 2021-10-26 1:42 UTC (permalink / raw)
To: jwi, bvanassche, martin.petersen, jejb, linux-scsi
Cc: gregkh, maier, bblock, linux-next, linux-s390
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-26 1:42 ` [PATCH v3] " Steffen Maier
@ 2021-10-26 10:00 ` Benjamin Block
2021-10-26 21:48 ` Bart Van Assche
2021-10-26 18:31 ` Bart Van Assche
2021-10-29 17:56 ` Benjamin Block
2 siblings, 1 reply; 14+ messages in thread
From: Benjamin Block @ 2021-10-26 10:00 UTC (permalink / raw)
To: Steffen Maier, bvanassche
Cc: jwi, martin.petersen, jejb, linux-scsi, gregkh, linux-next, linux-s390
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-26 1:42 ` [PATCH v3] " Steffen Maier
2021-10-26 10:00 ` Benjamin Block
@ 2021-10-26 18:31 ` Bart Van Assche
2021-10-29 17:56 ` Benjamin Block
2 siblings, 0 replies; 14+ messages in thread
From: Bart Van Assche @ 2021-10-26 18:31 UTC (permalink / raw)
To: Steffen Maier, jwi, martin.petersen, jejb, linux-scsi
Cc: gregkh, bblock, linux-next, linux-s390
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>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-26 10:00 ` Benjamin Block
@ 2021-10-26 21:48 ` Bart Van Assche
2021-10-29 17:55 ` Benjamin Block
0 siblings, 1 reply; 14+ messages in thread
From: Bart Van Assche @ 2021-10-26 21:48 UTC (permalink / raw)
To: Benjamin Block, Steffen Maier
Cc: jwi, martin.petersen, jejb, linux-scsi, gregkh, linux-next, linux-s390
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-26 21:48 ` Bart Van Assche
@ 2021-10-29 17:55 ` Benjamin Block
0 siblings, 0 replies; 14+ messages in thread
From: Benjamin Block @ 2021-10-29 17:55 UTC (permalink / raw)
To: Bart Van Assche
Cc: Steffen Maier, jwi, martin.petersen, jejb, linux-scsi, gregkh,
linux-next, linux-s390
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] scsi: core: Fix early registration of sysfs attributes for scsi_device
2021-10-26 1:42 ` [PATCH v3] " Steffen Maier
2021-10-26 10:00 ` Benjamin Block
2021-10-26 18:31 ` Bart Van Assche
@ 2021-10-29 17:56 ` Benjamin Block
2 siblings, 0 replies; 14+ messages in thread
From: Benjamin Block @ 2021-10-29 17:56 UTC (permalink / raw)
To: Steffen Maier
Cc: jwi, bvanassche, martin.petersen, jejb, linux-scsi, gregkh,
linux-next, linux-s390
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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-10-29 17:56 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20211012233558.4066756-1-bvanassche@acm.org>
[not found] ` <163478764102.7011.9375895285870786953.b4-ty@oracle.com>
2021-10-23 20:54 ` Missing driver-specific sysfs attributes of scsi_device [was: Re: [PATCH v4 00/46] Register SCSI sysfs attributes earlier] Steffen Maier
2021-10-24 0:39 ` Steffen Maier
2021-10-24 2:55 ` Bart Van Assche
2021-10-24 11:18 ` [PATCH] scsi: core: Fix early registration of sysfs attributes for scsi_device Steffen Maier
2021-10-24 21:25 ` Bart Van Assche
2021-10-24 22:16 ` [PATCH v2] " Steffen Maier
2021-10-25 9:23 ` Julian Wiedmann
2021-10-25 16:00 ` Bart Van Assche
2021-10-26 1:42 ` [PATCH v3] " Steffen Maier
2021-10-26 10:00 ` Benjamin Block
2021-10-26 21:48 ` Bart Van Assche
2021-10-29 17:55 ` Benjamin Block
2021-10-26 18:31 ` Bart Van Assche
2021-10-29 17:56 ` Benjamin Block
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).