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

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

On 9/11/23 19:38, John Garry wrote:
> On 11/09/2023 07:48, Damien Le Moal wrote:
>>>>   #define ATA_BASE_SHT(drv_name)                    \
>>> I do understand the rationale here, as the relationship between ata port and
>>> scsi devices are blurred. Question is: blurred by what?
>>> Is it the libata/SAS duality where SCSI devices will have a different
>>> ancestry for libata and libsas?
>>> If so, why don't we need the 'link' mechanism for SAS?
>>> Or is it something else?
>> On scsi, ancestry from Scsi_Host is clear: host->target->device->disk.
>> For ATA, it is also clear: port->link->device
>>
>> The relationship is that ata port has its own Scsi_Host, but no "family"
>> relationship here. They are just "friends", so scsi disk and ata_port have no
>> direct ancestry. Adding the link creates that.
>>
>> libsas does not need the link because it does its own PM management of the
>> ports, not relying on PM to order things.
>>
> 
> 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 :)

> I think that this should really be added for all libsas drivers which 
> support RPM - pm8001 is missing, IIRC.

Will try again tomorrow looking at hisi driver for inspiration. The other thing
I need to better understand is how the suspend & resume operations of libsas get
triggered. For suspend, it is easy-ish, but for resume, it seems to be tightly
coupled with discovery, so the the adapter resume (scsi host class resume ?).
If that is the case, then that is done *before* the scsi_dev resume because of
the existing device link dependencies. So I am not yet entirely convinced that
adding a link between ata_port and sdev will serve any purpose now that the
asynchronous libata resume is fixed and synchronized with the scsi disk
resume... Will dig further.

That said, it may be good to move libsas suspend/resume fixes to another patch
series. There is an outstanding regression/problem for pure libata devices that
this series fixes. So would like to get the fixes patches in Linus tree ASAP.

Cheers.

-- 
Damien Le Moal
Western Digital Research