Re: [PATCH] Revert "scsi: pm80xx: Do not use libsas port ID"

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

On 2025/07/18 6:20, Igor Pylypiv wrote:
> On Thu, Jul 17, 2025 at 06:56:07PM +0200, Niklas Cassel wrote:
>> This reverts commit 0f630c58e31afb3dc2373bc1126b555f4b480bb2.
>>
>> Commit 0f630c58e31a ("scsi: pm80xx: Do not use libsas port ID") causes
>> drives behind a SAS enclosure (which is connected to one of the ports
>> on the HBA) to no longer be detected.
>>
>> Connecting the drives directly to the HBA still works. Thus, the commit
>> only broke the detection of drives behind a SAS enclosure.
>>
>> Reverting the commit makes the drives behind the SAS enclosure to be
>> detected once again.
>>
>> The commit log of the offending commit is quite vague, but mentions that:
>> "Remove sas_find_local_port_id(). We can use pm8001_ha->phy[phy_id].port
>> to get the port ID."
>>
>> This assumption appears false, thus revert the offending commit.
> 
> Thanks for bisecting and reverting the commit, Niklas!
> 
> Let me review the changes and send a proper fix that takes into account
> drives behind a SAS enclosure. I would appretiate if you could test that
> new fix since I don't have a setup with a SAS enclosure.

s/enclosure/expander
(the important point here is if there is an expander between the HBA and the
devices, not how the devices are installed. E.g. a simple enclosure may not have
any expander and act similar to a fan-out cable connection)

I think the issue is that if you do not have an expander and use fan-out cables
to connect drives directly to the HBA, you essentially get HBA SAS port ==
device port and it works (My setup is like this with an -8e model, so 8 ports, 0
to 7). That is the case for "if (!pdev)" in sas_find_local_port.

But if there is an expander, you can have multiple devices per HBA port, so you
need to search backward using the parent device until you hit the HBA device
itself, which gives you the port to use to communicate with the device.

So not sure if we can simplify/remove the loop in sas_find_local_port(). Maybe
if we add "local_port" field to struct domain_device ? But then any topology
change event would potentially need to update this.


-- 
Damien Le Moal
Western Digital Research