On Wed, Sep 20, 2023 at 10:54:19PM +0900, Damien Le Moal wrote:
> There is no direct device ancestry defined between an ata_device and
> its scsi device which prevents the power management code from correctly
> ordering suspend and resume operations. Create such ancestry with the
> ata device as the parent to ensure that the scsi device (child) is
> suspended before the ata device and that resume handles the ata device
> before the scsi device.
>
> The parent-child (supplier-consumer) relationship is established between
> the ata_port (parent) and the scsi device (child) with the function
> device_add_link(). The parent used is not the ata_device as the PM
> operations are defined per port and the status of all devices connected
> through that port is controlled from the port operations.
>
> The device link is established with the new function
> ata_scsi_dev_alloc(). This function is used to define the ->slave_alloc
> callback of the scsi host template of most drivers.
>
> Fixes: a19a93e4c6a9 ("scsi: core: pm: Rely on the device driver core for async power management")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> ---
> drivers/ata/libata-scsi.c | 49 +++++++++++++++++++++++++++++++++++----
> drivers/ata/libata.h | 1 +
> include/linux/libata.h | 2 ++
> 3 files changed, 47 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index fb73c145b49a..f420b65d3331 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1089,6 +1089,46 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
> return 0;
> }
>
> +int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap)
> +{
> + struct device_link *link;
> +
> + ata_scsi_sdev_config(sdev);
> +
> + /*
> + * Create a link from the ata_port device to the scsi device to ensure
> + * that PM does suspend/resume in the correct order: the scsi device is
> + * consumer (child) and the ata port the supplier (parent).
> + */
> + link = device_link_add(&sdev->sdev_gendev, &ap->tdev,
> + DL_FLAG_STATELESS |
> + DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
> + if (!link) {
> + ata_port_err(ap, "Failed to create link to scsi device %s\n",
> + dev_name(&sdev->sdev_gendev));
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ata_scsi_slave_alloc - Early setup of SCSI device
> + * @sdev: SCSI device to examine
> + *
> + * This is called from scsi_alloc_sdev() when the scsi device
> + * associated with an ATA device is scanned on a port.
> + *
> + * LOCKING:
> + * Defined by SCSI layer. We don't really care.
> + */
> +
> +int ata_scsi_slave_alloc(struct scsi_device *sdev)
> +{
> + return ata_scsi_dev_alloc(sdev, ata_shost_to_port(sdev->host));
> +}
> +EXPORT_SYMBOL_GPL(ata_scsi_slave_alloc);
> +
> /**
> * ata_scsi_slave_config - Set SCSI device attributes
> * @sdev: SCSI device to examine
> @@ -1105,14 +1145,11 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
> {
> struct ata_port *ap = ata_shost_to_port(sdev->host);
> struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
> - int rc = 0;
> -
> - ata_scsi_sdev_config(sdev);
>
> if (dev)
> - rc = ata_scsi_dev_config(sdev, dev);
> + return ata_scsi_dev_config(sdev, dev);
>
> - return rc;
> + return 0;
> }
> EXPORT_SYMBOL_GPL(ata_scsi_slave_config);
>
> @@ -1136,6 +1173,8 @@ void ata_scsi_slave_destroy(struct scsi_device *sdev)
> unsigned long flags;
> struct ata_device *dev;
>
> + device_link_remove(&sdev->sdev_gendev, &ap->tdev);
> +
> spin_lock_irqsave(ap->lock, flags);
> dev = __ata_scsi_find_dev(ap, sdev);
> if (dev && dev->sdev) {
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 6e7d352803bd..079981e7156a 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -111,6 +111,7 @@ extern struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
> extern int ata_scsi_add_hosts(struct ata_host *host,
> const struct scsi_host_template *sht);
> extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
> +extern int ata_scsi_dev_alloc(struct scsi_device *sdev, struct ata_port *ap);
> extern int ata_scsi_offline_dev(struct ata_device *dev);
> extern bool ata_scsi_sense_is_valid(u8 sk, u8 asc, u8 ascq);
> extern void ata_scsi_set_sense(struct ata_device *dev,
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index bf4913f4d7ac..4ece1b7a2a5b 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1148,6 +1148,7 @@ extern int ata_std_bios_param(struct scsi_device *sdev,
> struct block_device *bdev,
> sector_t capacity, int geom[]);
> extern void ata_scsi_unlock_native_capacity(struct scsi_device *sdev);
> +extern int ata_scsi_slave_alloc(struct scsi_device *sdev);
> extern int ata_scsi_slave_config(struct scsi_device *sdev);
> extern void ata_scsi_slave_destroy(struct scsi_device *sdev);
> extern int ata_scsi_change_queue_depth(struct scsi_device *sdev,
> @@ -1396,6 +1397,7 @@ extern const struct attribute_group *ata_common_sdev_groups[];
> .this_id = ATA_SHT_THIS_ID, \
> .emulated = ATA_SHT_EMULATED, \
> .proc_name = drv_name, \
> + .slave_alloc = ata_scsi_slave_alloc, \
> .slave_destroy = ata_scsi_slave_destroy, \
> .bios_param = ata_std_bios_param, \
> .unlock_native_capacity = ata_scsi_unlock_native_capacity,\
> --
> 2.41.0
>
Reviewed-by: Niklas Cassel <niklas.cassel@xxxxxxx>
