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

[Damien Le Moal <dlemoal@kernel.org>]

On 9/12/23 00:15, John Garry wrote:
> On 11/09/2023 12:48, Damien Le Moal wrote:
>>> For hisi_sas v3, RPM is supported and a link was required to be added in
>>> that driver between the sdev and those host controller device - see
>>> 16fd4a7c59. And that is for a similar reason here. I see that we already
>>> have an ancestry for libata between ata_dev -> ata_link -> ata_port ->
>>> ata_host dev, so it seems reasonable to add ata_dev  <-> sdev dependency
>>> here.
>> libata creates a link between ata_port and sdev. This is not very natural, but
>> as I said in the cover letter, the power management model is weird as everything
>> is per port, even if a port has multiple devices. Nevertheless, I tried to apply
>> the same for libsas but failed: if I add the link creation in the slave_alloc
>> method, I fail to get the scsi disks to show up (no /dev/sdX device files). Not
>> sure why. That was with my pm8001 adapter, which is the only libsas one I have.
>>
>> Side note: having an ata_debug (ata equivalent of scsi_debug) driver that could
>> act as a pure libata driver or as a libsas adapter would really be awesome for
>> this kind of tests 😄
> 
> hmm... maybe scsi_debug could be modified to support ATA devices (with 
> libata).
> 
> Regardless, I'm happy to check the code change which you attempted if 
> shared.

I had a closer look at what the hisi_sas driver is doing. The link it creates
with device_add_link() is between the scsi device gendev and the hisi_hba->dev,
that is, the HBA device. So nothing to do with libata port of device.

Trying to mimic what libata is now doing, I tried this:

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 12e2653846e3..91d76258e6ea 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -624,6 +624,26 @@ int sas_ata_init(struct domain_device *found_dev)
        return rc;
 }

+int sas_ata_slave_configure(struct scsi_device *sdev)
+{
+       struct domain_device *dev = sdev_to_domain_dev(sdev);
+       struct ata_port *ap = dev->sata_dev.ap;
+       struct device *sdev_dev = &sdev->sdev_gendev;
+       struct device_link *link;
+
+       ata_sas_slave_configure(sdev, ap);
+
+       link = device_link_add(sdev_dev, &ap->tdev,
+                              DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
+       if (!link && pm_runtime_enabled(sdev_dev)) {
+               dev_err(sdev_dev,
+                       "add device link failed, disabling runtime PM for the
host\n");
+               pm_runtime_disable(sdev_dev);
+       }
+
+       return 0;
+}
+
 void sas_ata_task_abort(struct sas_task *task)
 {
        struct ata_queued_cmd *qc = task->uldd_task;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index a6dc7dc07fce..82c6d8e1e8c7 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -58,6 +58,8 @@ void sas_enable_revalidation(struct sas_ha_struct *ha);
 void sas_queue_deferred_work(struct sas_ha_struct *ha);
 void __sas_drain_work(struct sas_ha_struct *ha);

+int sas_ata_slave_configure(struct scsi_device *sdev);
+
 void sas_deform_port(struct asd_sas_phy *phy, int gone);

 void sas_porte_bytes_dmaed(struct work_struct *work);
diff --git a/drivers/scsi/libsas/sas_scsi_host.c
b/drivers/scsi/libsas/sas_scsi_host.c
index 9047cfcd1072..b6c419aa563e 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -810,10 +810,8 @@ int sas_slave_configure(struct scsi_device *scsi_dev)

        BUG_ON(dev->rphy->identify.device_type != SAS_END_DEVICE);

-       if (dev_is_sata(dev)) {
-               ata_sas_slave_configure(scsi_dev, dev->sata_dev.ap);
-               return 0;
-       }
+       if (dev_is_sata(dev))
+               return sas_ata_slave_configure(scsi_dev);

        sas_read_port_mode_page(scsi_dev);

This does not change the drive discovery, all drives connected to the HBA are
found and identified:

[   52.974154] scsi host12: pm80xx
[   54.445986] sas: phy-12:4 added to port-12:0, phy_mask:0x10 (50010860002f5644)
[   54.449019] sas: DOING DISCOVERY on port 0, pid:423
[   54.461108] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[   54.465601] sas: ata13: end_device-12:0: dev error handler
[   54.618588] ata13.00: ATA-11: WDC  WUH721818ALN604, PCGNW232, max UDMA/133
[   56.295486] ata13.00: 4394582016 sectors, multi 0: LBA48 NCQ (depth 32)
[   56.306849] ata13.00: Features: NCQ-sndrcv NCQ-prio
[   56.320849] ata13.00: configured for UDMA/133
[   56.324504] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1
[   56.340346] scsi 12:0:0:0: Direct-Access     ATA      WDC  WUH721818AL W232
PQ: 0 ANSI: 5
[   56.347160] sas: DONE DISCOVERY on port 0, pid:423, result:0
[   56.348161] sas: phy-12:5 added to port-12:1, phy_mask:0x20 (50010860002f5645)
[   56.351291] sas: DOING DISCOVERY on port 1, pid:423
[   56.363285] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[   56.368788] sas: ata13: end_device-12:0: dev error handler
[   56.368859] sas: ata14: end_device-12:1: dev error handler
[   56.522903] ata14.00: ATA-11: WDC  WUH721818ALN604, PCGNWTW2, max UDMA/133
[   58.402015] ata14.00: 4394582016 sectors, multi 0: LBA48 NCQ (depth 32)
[   58.406207] ata14.00: Features: NCQ-sndrcv NCQ-prio
[   58.420254] ata14.00: configured for UDMA/133
[   58.423222] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1
[   58.438869] scsi 12:0:1:0: Direct-Access     ATA      WDC  WUH721818AL WTW2
PQ: 0 ANSI: 5
[   58.445619] sas: DONE DISCOVERY on port 1, pid:423, result:0
[   58.446687] sas: phy-12:6 added to port-12:2, phy_mask:0x40 (50010860002f5646)
[   58.449803] sas: DOING DISCOVERY on port 2, pid:423
[   58.461732] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[   58.465369] sas: ata13: end_device-12:0: dev error handler
[   58.465426] sas: ata14: end_device-12:1: dev error handler
[   58.465432] sas: ata15: end_device-12:2: dev error handler
[   58.622345] ata15.00: ATA-12: WDC  WUH722222ALN604, LNGNW1TZ, max UDMA/133
[   59.540468] ata15.00: 5371330560 sectors, multi 0: LBA48 NCQ (depth 32)
[   59.588217] ata15.00: Features: NCQ-sndrcv NCQ-prio CDL
[   59.697588] ata15.00: configured for UDMA/133
[   59.700514] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1
[   59.716067] scsi 12:0:2:0: Direct-Access     ATA      WDC  WUH722222AL W1TZ
PQ: 0 ANSI: 5
[   59.723838] sas: DONE DISCOVERY on port 2, pid:423, result:0
[   59.724893] sas: phy-12:7 added to port-12:3, phy_mask:0x80 (50010860002f5647)
[   59.727852] sas: DOING DISCOVERY on port 3, pid:423
[   59.739699] sas: Enter sas_scsi_recover_host busy: 0 failed: 0
[   59.743344] sas: ata13: end_device-12:0: dev error handler
[   59.743402] sas: ata14: end_device-12:1: dev error handler
[   59.743405] sas: ata15: end_device-12:2: dev error handler
[   59.743429] sas: ata16: end_device-12:3: dev error handler
[   59.898839] ata16.00: ATA-11: WDC  WSH722020ALN604, PCGMW803, max UDMA/133
[   62.348080] ata16.00: 4882956288 sectors, multi 0: LBA48 NCQ (depth 32)
[   62.353731] ata16.00: Features: NCQ-sndrcv NCQ-prio
[   62.370011] ata16.00: configured for UDMA/133
[   62.373532] sas: --- Exit sas_scsi_recover_host: busy: 0 failed: 0 tries: 1
[   62.389238] scsi 12:0:3:0: Direct-Access-ZBC ATA      WDC  WSH722020AL W803
PQ: 0 ANSI: 7
[   62.396642] sas: DONE DISCOVERY on port 3, pid:423, result:0

But no scsi disk added:

# lsscsi
[12:0:0:0]   disk    ATA      WDC  WUH721818AL W232  -
[12:0:1:0]   disk    ATA      WDC  WUH721818AL WTW2  -
[12:0:2:0]   disk    ATA      WDC  WUH722222AL W1TZ  -
[12:0:3:0]   zbc     ATA      WDC  WSH722020AL W803  -

(no /dev/sdX device).
While not printed on this run, I often see messages like

scsi 12:0:0:0: deferred probe error

No idea why... I do not see where that comes from.

This is all with pm8001 driver.

-- 
Damien Le Moal
Western Digital Research