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

[Julian Wiedmann <jwi@linux.ibm.com>]

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