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

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

+

On 12/09/2023 10:00, Damien Le Moal wrote:
>>> +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.
> For libata, the point is to have PM order suspend resume operations correctly:
> suspend: scsi dev first, then ata_port
> resume: ata_port first then scsi dev
> 
> Strictly speaking, we should do that with ata_dev and scsi dev, but libata PM
> operations are defined for ata_port, not ata_dev.
> 
> For libsas, the PM operations come from scsi, so scsi bus operations for
> devices. And given that:
> 1) we should already have the dev chain:
> hba dev -> scsi host ->scsi target -> scsi dev

That would seem right, since scsi_add_host() is passed the host dev and 
the ancestry would be created naturally, but for hisi_sas a link was 
still required. Let me check why that is again ...

> 2) libsas does its own ata PM control when suspending & resuming the HBA (if I
> understand things correctly)
> 
> we should thus not need anything special for libsas. ata devices suspend/resume
> will be done in the right order without an extra link. I do not really
> understand why hisi_sas need to create that link. If it is indeed needed, then
> we probably should have it created always by libsas generic code...

Thanks,
John