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

[Igor Pylypiv <ipylypiv@google.com>]

On Fri, Jul 18, 2025 at 01:35:13PM +0900, Damien Le Moal wrote:
> 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.
>

Looks like there is no need for sas_find_local_port() because child devices
have a reference to the same port?

struct domain_device {
...
	struct asd_sas_port *port;        /* shortcut to root of the tree */
}

https://github.com/torvalds/linux/blob/c7de79e662b8681f54196c107281f1e63c26a3db/include/scsi/libsas.h#L168

During discovery, child devices inherit parent's port:

https://github.com/torvalds/linux/blob/c7de79e662b8681f54196c107281f1e63c26a3db/drivers/scsi/libsas/sas_expander.c#L826

and also

https://github.com/torvalds/linux/blob/c7de79e662b8681f54196c107281f1e63c26a3db/drivers/scsi/libsas/sas_expander.c#L936

Hey Niklas,

Could you try the following fix with your expander setup, please?
The fix assumes the problematic patch is not yet revered.

$ git diff
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index f7067878b34f..cd9513c23c71 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -503,7 +503,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
        spin_lock_irqsave(&pm8001_ha->lock, flags);
 
        pm8001_dev = dev->lldd_dev;
-       port = pm8001_ha->phy[pm8001_dev->attached_phy].port;
+       port = dev->port->lldd_port;
 
        if (!internal_abort &&
            (DEV_IS_GONE(pm8001_dev) || !port || !port->port_attached)) {


Thank you!
Igor

> 
> -- 
> Damien Le Moal
> Western Digital Research