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 09:44, Li, Eric (Honggang) wrote:
> Resend this email.
> 
>> -----Original Message-----
>> From: Li, Eric (Honggang)
>> Sent: Monday, May 6, 2024 9:50 AM
>> To: John Garry <john.g.garry@oracle.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
>>
>>> -----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.

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?



>> 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()

> 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.)
>>ok, this can be considered more when I understand exactly what you have 
and what else you think may be broken.

Thanks,
John