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

Hannes Reinecke <hare@xxxxxxx> · Mon, 11 Sep 2023 09:07:03 +0200

On 9/11/23 08:48, Damien Le Moal wrote:
On 9/11/23 15:41, Hannes Reinecke wrote:
On 9/11/23 06:02, 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>
---
   drivers/ata/libata-scsi.c | 46 ++++++++++++++++++++++++++++++++++-----
   drivers/ata/libata.h      |  1 +
   drivers/ata/pata_macio.c  |  1 +
   drivers/ata/sata_mv.c     |  1 +
   drivers/ata/sata_nv.c     |  2 ++
   drivers/ata/sata_sil24.c  |  1 +
   include/linux/libata.h    |  3 +++
   7 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d3f28b82c97b..f63cf6e7332e 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1089,6 +1089,45 @@ 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_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 +1144,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);
   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/drivers/ata/pata_macio.c b/drivers/ata/pata_macio.c
index 17f6ccee53c7..32968b4cf8e4 100644
--- a/drivers/ata/pata_macio.c
+++ b/drivers/ata/pata_macio.c
@@ -918,6 +918,7 @@ static const struct scsi_host_template pata_macio_sht = {
        * use 64K minus 256
        */
       .max_segment_size    = MAX_DBDMA_SEG,
+    .slave_alloc        = ata_scsi_slave_alloc,
       .slave_configure    = pata_macio_slave_config,
       .sdev_groups        = ata_common_sdev_groups,
       .can_queue        = ATA_DEF_QUEUE,
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index d105db5c7d81..353ac7b2f14a 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -673,6 +673,7 @@ static const struct scsi_host_template mv6_sht = {
       .sdev_groups        = ata_ncq_sdev_groups,
       .change_queue_depth    = ata_scsi_change_queue_depth,
       .tag_alloc_policy    = BLK_TAG_ALLOC_RR,
+    .slave_alloc        = ata_scsi_slave_alloc,
       .slave_configure    = ata_scsi_slave_config
   };
   diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 0a0cee755bde..5428dc2ec5e3 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -380,6 +380,7 @@ static const struct scsi_host_template nv_adma_sht = {
       .can_queue        = NV_ADMA_MAX_CPBS,
       .sg_tablesize        = NV_ADMA_SGTBL_TOTAL_LEN,
       .dma_boundary        = NV_ADMA_DMA_BOUNDARY,
+    .slave_alloc        = ata_scsi_slave_alloc,
       .slave_configure    = nv_adma_slave_config,
       .sdev_groups        = ata_ncq_sdev_groups,
       .change_queue_depth     = ata_scsi_change_queue_depth,
@@ -391,6 +392,7 @@ static const struct scsi_host_template nv_swncq_sht = {
       .can_queue        = ATA_MAX_QUEUE - 1,
       .sg_tablesize        = LIBATA_MAX_PRD,
       .dma_boundary        = ATA_DMA_BOUNDARY,
+    .slave_alloc        = ata_scsi_slave_alloc,
       .slave_configure    = nv_swncq_slave_config,
       .sdev_groups        = ata_ncq_sdev_groups,
       .change_queue_depth     = ata_scsi_change_queue_depth,
diff --git a/drivers/ata/sata_sil24.c b/drivers/ata/sata_sil24.c
index 142e70bfc498..e0b1b3625031 100644
--- a/drivers/ata/sata_sil24.c
+++ b/drivers/ata/sata_sil24.c
@@ -381,6 +381,7 @@ static const struct scsi_host_template sil24_sht = {
       .tag_alloc_policy    = BLK_TAG_ALLOC_FIFO,
       .sdev_groups        = ata_ncq_sdev_groups,
       .change_queue_depth    = ata_scsi_change_queue_depth,
+    .slave_alloc        = ata_scsi_slave_alloc,
       .slave_configure    = ata_scsi_slave_config
   };
   diff --git a/include/linux/libata.h b/include/linux/libata.h
index 52d58b13e5ee..c8cfea386c16 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1144,6 +1144,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,
@@ -1401,12 +1402,14 @@ extern const struct attribute_group
*ata_common_sdev_groups[];
       __ATA_BASE_SHT(drv_name),                \
       .can_queue        = ATA_DEF_QUEUE,        \
       .tag_alloc_policy    = BLK_TAG_ALLOC_RR,        \
+    .slave_alloc        = ata_scsi_slave_alloc,        \
       .slave_configure    = ata_scsi_slave_config
     #define ATA_SUBBASE_SHT_QD(drv_name, drv_qd)            \
       __ATA_BASE_SHT(drv_name),                \
       .can_queue        = drv_qd,            \
       .tag_alloc_policy    = BLK_TAG_ALLOC_RR,        \
+    .slave_alloc        = ata_scsi_slave_alloc,        \
       .slave_configure    = ata_scsi_slave_config
     #define ATA_BASE_SHT(drv_name)                    \
I do understand the rationale here, as the relationship between ata port and
scsi devices are blurred. Question is: blurred by what?
Is it the libata/SAS duality where SCSI devices will have a different
ancestry for libata and libsas?
If so, why don't we need the 'link' mechanism for SAS?
Or is it something else?

On scsi, ancestry from Scsi_Host is clear: host->target->device->disk.
For ATA, it is also clear: port->link->device

The relationship is that ata port has its own Scsi_Host, but no "family"
relationship here. They are just "friends", so scsi disk and ata_port have no
direct ancestry. Adding the link creates that.

libsas does not need the link because it does its own PM management of the
ports, not relying on PM to order things.

Okay.

Reviewed-by: Hannes Reinecke <hare@xxxxxxx>

Cheers,

Hannes
--
Dr. Hannes Reinecke                Kernel Storage Architect
hare@xxxxxxx                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman







