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
