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

[John Garry <john.g.garry@oracle.com>]

On 07/05/2024 12:17, Li, Eric (Honggang) wrote:
>> Sure, I don't particularly like it as a fix either. But first I would like to get your setup working
>> again to verify that only this needs fixing. Indeed something else may be broken since
>> a1b6fb947f923. In addition, if we need to backport a fix, I would only like to backport a fix for
>> real known broken topologies, and not theoretical issues not experienced.
>>
>> But what exact setup do you have? I am confused, as you seem to be talking about your
>> setup being broken, but then other setup may also being broken, but you don't have access
>> to another setup. What is the other setup?
>>
> This issue was reported by our tester. His setup is
> SAS Controller <--> SAS Expander <--> SAS Expander <--> SAS drives
>                                                                     <--> SAS Expander <--> SAS drives
>                                                                     ......
> When this issue occurred, no SAS drives could be detected.
> As a workaround, I've already added those codes removed by the commit a1b6fb947f923, and at least we could detect the SAS drives.

ok, good to know, but it would be good to confirm that just re-adding 
the code to set phy_state is enough.

> 
> Since our SAS expanders are self-configured, we don't need to explicitly configure the per-PHY routing tables.
> So, I am not sure there is any other issue in this workaround as some per-PHY routing tables are not configured.
> 
>>>> 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,
>> I would have thought that re-adding the code removed in a1b6fb947f923 to set
>> PHY_STATE_DISCOVERED would only affect the first phy scanned in that wideport. Other
>> phys scanned would have it set through calls to
>> sas_ex_join_wide_port()
>>
> I don't catch your point.
> In my understanding, it would affect the rest PHYs in that wide port, not the first one.
> The first PHY has been scanned/discovered and added to a port (at that time, it is just a narrow port yet).

Agreed

> Call to sas_ex_join_wide_port() makes the rest PHYs associated with that existing port (making it become wideport) and set up sysfs between the PHY and port. > Set PHY_STATE_DISCOVERED would make the rest PHYs not being scanned/discovered again (as this wide port is already scanned).

If you can just confirm that re-adding the code to set phy_state = 
DISCOVERED is good enough to see the SAS disks again, then this can be 
further discussed.

Thanks,
John