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

[Benjamin Block <bblock@linux.ibm.com>]

On Tue, Oct 26, 2021 at 03:42:40AM +0200, Steffen Maier wrote:
> v4.17 commit 86b87cde0b55 ("scsi: core: host template attribute groups")
> introduced explicit sysfs_create_groups() in scsi_sysfs_add_sdev()
> and sysfs_remove_groups() in __scsi_remove_device(), both for sdev_gendev,
> based on a new field const struct attribute_group **sdev_groups
> of struct scsi_host_template.
> 
> Commit 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
> removed above explicit (de)registration of scsi_device attribute groups.
> It also converted all scsi_device attributes and attribute_groups to
> end up in a new field const struct attribute_group *gendev_attr_groups[6]
> of struct scsi_device. However, that new field was not used anywhere.
> 
> Surprisingly, this only caused missing LLDD specific scsi_device sysfs
> attributes. Whereas, scsi core attributes from scsi_sdev_attr_groups
> did continue to exist because of scsi_dev_type.groups.
> 
> We separate scsi core attibutes from LLDD specific attributes.
> Hence, we keep the initializing assignment scsi_dev_type =
> { .groups = scsi_sdev_attr_groups, } as this takes care of core
> attributes. Without the separation, it would cause attribute double
> registration due to scsi_dev_type.groups and sdev_gendev.groups.
> 
> Julian suggested to assign the sdev_groups pointer of the
> scsi_host_template directly to the groups pointer of sdev_gendev.
> This way we can delete the container scsi_device.gendev_attr_groups
> and the loop copying each entry from hostt->sdev_groups to
> sdev->gendev_attr_groups.
> 
> Alternative approaches ruled out:
> Assigning gendev_attr_groups to sdev_dev has no visible effect.
> Assigning sdev->gendev_attr_groups to scsi_dev_type.groups
> caused scsi_device of all scsi host types to get LLDD specific
> attributes of the LLDD for which the last sdev alloc happened to occur,
> as that overwrote scsi_dev_type.groups,
> e.g. scsi_debug had zfcp-specific scsi_device attributes.
> 
> Signed-off-by: Steffen Maier <maier@linux.ibm.com>
> Fixes: 92c4b58b15c5 ("scsi: core: Register sysfs attributes earlier")
> Suggested-by: Julian Wiedmann <jwi@linux.ibm.com>
> ---
> 
> Notes:
>     Changes in v3:
>     * integrated Julian's feedback of dropping detour through
>       gendev_attr_groups
>     
>     Changes in v2:
>     * integrated Bart's feedback of updating the comment for
>       the gendev_attr_groups declaration to match the code change
>     * in that spirit also adapted the vector size of that field
>     * eliminated the now unnecessary second loop counter 'j'
> 
>  drivers/scsi/scsi_sysfs.c  | 11 +----------
>  include/scsi/scsi_device.h |  6 ------
>  2 files changed, 1 insertion(+), 16 deletions(-)
> 

Looks good to me.

Reviewed-by: Benjamin Block <bblock@linux.ibm.com>

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