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

[John Garry <john.g.garry@oracle.com>]

On 12/09/2023 07:13, Damien Le Moal wrote:
>> 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.

Correct, it's just for the sdev -> host dev link

> 
> 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);

I am not sure what is the point of this. For libsas/pm8001 driver, there 
is no ata_port -> .. -> host dev link, right? If so, it just seems that 
you are are adding a dependency to a device (&ap->tdev) which has no 
dependency to a real device itself.

> +       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.

It might be related to what I mention, above. Anyway, I would rather you 
don't get this series bogged down in libsas issues.

> 
> This is all with pm8001 driver.

Thanks,
John