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

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

On 9/12/23 17:49, John Garry 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
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...

-- 
Damien Le Moal
Western Digital Research