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