Re: [PATCH v4 03/23] ata: libata-scsi: link ata port and scsi device

[Niklas Cassel <Niklas.Cassel@wdc.com>]

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@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> ---
>  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@wdc.com>