Re: [PATCH 1/2] scsi: Register SCSI host sysfs attributes earlier

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Fri, Sep 24, 2021 at 04:26:34PM -0700, Bart Van Assche wrote:
> A quote from Documentation/driver-api/driver-model/device.rst:
> "Word of warning:  While the kernel allows device_create_file() and
> device_remove_file() to be called on a device at any time, userspace has
> strict expectations on when attributes get created.  When a new device is
> registered in the kernel, a uevent is generated to notify userspace (like
> udev) that a new device is available.  If attributes are added after the
> device is registered, then userspace won't get notified and userspace will
> not know about the new attributes."
> 
> Hence register SCSI host sysfs attributes before the SCSI host shost_dev
> uevent is emitted instead of after that event has been emitted.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Nice work, this is good to see.

Tiny comments below:

> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 3f6f14f0cafb..f424aca6dc6e 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -480,7 +480,15 @@ 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 = scsi_sysfs_shost_attr_groups;
> +	shost->shost_dev.groups = shost->shost_dev_attr_groups;
> +	shost->shost_dev_attr_groups[0] = &scsi_host_attr_group;
> +	if (shost->hostt->shost_attrs) {
> +		shost->shost_dev_attr_groups[1] =
> +			&shost->shost_driver_attr_group;
> +		shost->shost_driver_attr_group = (struct attribute_group){
> +			.attrs = shost->hostt->shost_attrs,
> +		};

Did you just allocate this off the stack?  What happens when the
function returns and the stack data is returned?

> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -476,7 +476,7 @@ struct scsi_host_template {
>  	/*
>  	 * Pointer to the sysfs class properties for this host, NULL terminated.
>  	 */
> -	struct device_attribute **shost_attrs;
> +	struct attribute **shost_attrs;

Why isn't this "struct attribute_group **groups"?

>  
>  	/*
>  	 * Pointer to the SCSI device properties for this host, NULL terminated.
> @@ -695,6 +695,8 @@ struct Scsi_Host {
>  
>  	/* ldm bits */
>  	struct device		shost_gendev, shost_dev;
> +	struct attribute_group  shost_driver_attr_group;
> +	const struct attribute_group *shost_dev_attr_groups[3];

Why just 3?

thanks,

greg k-h