RE: Issue in sas_ex_discover_dev() for multiple level of SAS expanders in a domain

Li, Eric (Honggang)
2024-05-06T01:50:00Z

Internal Use - Confidential
> -----Original Message-----
> From: John Garry <john.g.garry@oracle.com>
> Sent: Friday, May 3, 2024 4:33 PM
> To: Li, Eric (Honggang) <Eric.H.Li@Dell.com>; Jason Yan <yanaijie@huawei.com>;
> james.bottomley@hansenpartnership.com; Martin K . Petersen <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: Issue in sas_ex_discover_dev() for multiple level of SAS expanders in a domain
>
>
> [EXTERNAL EMAIL]
>
> On 03/05/2024 04:15, Li, Eric (Honggang) wrote:
> > John,
> >
> > I agree that the call to sas_ex_join_wide_port() is not mandatory. In
> > fact, some logic here is similar to that function. We don't need to do
> > it again.
> > But just updating the phy_state may not be enough. I suppose you still
> > need to add that PHY into the corresponding wide port by calling
> > sas_port_add_phy() and update phy->port.
> > Just updating the phy_state may avoid the port disabled in this issue,
> > but other missing piece of work may cause other issues.
> >
>
> If you check the commit in which that call to sas_ex_join_wide_port() was originally added -
> 19252de - it was added together with the comment "Due to races, the phy might not get
> added to the wide port, so we add the phy to the wide port here". However the code to set
> phy_state = PHY_STATE_DISCOVERED already existed before that commit.
>
> When all that code was removed in a1b6fb947f923, I am just wondering if we have kept the
> phy_state = PHY_STATE_DISCOVERED code.
>
> Anyway, would we really join a wideport with the downstream expander? I would just
> assume that we would be creating a new wideport, and a subsequent scanned phy would be
> added to it.

[Eric: ] I don't think the code removed in a1b6fb947f923 is the right way to fix this issue.
It just happened avoiding this issue occurring.
I think the root cause of this issue is the order of function calls to sas_dev_present_in_domain() and sas_ex_join_wide_port().
My concern here is whether or not we still need to configure routing on the parent SAS expander before calling sas_ex_join_wide_port().
This part of logic is not present previously and I don't have environment to test this.

Back to your question, I believe we do need to join a wideport to downstream expander.
Changing the phy_state to PHY_STATE_DISCOVERED will skip scanning those PHYs, by not calling the function sas_ex_discover_dev() to subsequential PHYs within this port (so this issue wouldn’t hit). But those PHYs are not associated with a SAS port. This may trigger other issues (for example, any change count changed on that PHY, or SAS topology in sysfs, etc.)

>
>
> > Eric Li
> >