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

Li, Eric (Honggang)
2024-05-03T03:15:49Z

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.

Eric Li

Internal Use - Confidential
> -----Original Message-----
> From: John Garry <john.g.garry@oracle.com>
> Sent: Wednesday, May 1, 2024 10:24 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 30/04/2024 15:22, Li, Eric (Honggang) wrote:
> > I suppose you have got the log file I attached.
> > If not, please let me know.
> > Any update about this?
> >
> > Eric LI
>
> So if you revert a1b6fb947f923, but then remove the call to
> sas_ex_join_wide_port() re-added in that revert, is it ok? I am just wondering are
> we just missing the call to set phy_state = PHY_DEVICE_DISCOVERED after v5.3?
>
> Thanks,
> John
>
> >
> >
> > Internal Use - Confidential
> >> -----Original Message-----
> >> From: Li, Eric (Honggang)
> >> Sent: Thursday, April 25, 2024 1:04 PM
> >> To: Jason Yan <yanaijie@huawei.com>; John Garry
> >> <john.g.garry@oracle.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: Jason Yan <yanaijie@huawei.com>
> >>> Sent: Thursday, April 25, 2024 10:58 AM
> >>> To: John Garry <john.g.garry@oracle.com>; Li, Eric (Honggang)
> >>> <Eric.H.Li@Dell.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 2024/4/24 18:46, John Garry wrote:
> >>>> On 24/04/2024 09:59, Li, Eric (Honggang) wrote:
> >>>>> Hi,
> >>>>>
> >>>>> There is an issue in the function sas_ex_discover_dev() when I
> >>>>> have multiple SAS expanders chained under one SAS port on SAS
> controller.
> >>>>
> >>>> I think typically we can't and so don't test such a setup.
> >>>
> >>> Eric,
> >>>
> >>> I also don't understand why you need such a setup. Can you explain
> >>> more details of your topology?
> >>
> >> I believe this is common setup if you want to support large number of
> >> drives under one SAS port of SAS controller.
> >>
> >>>
> >>>>
> >>>>>
> >>>>> In this function, we first check whether the PHY’s
> >>>>> attached_sas_address is already present in the SAS domain, and
> >>>>> then check if this PHY belongs to an existing port on this SAS expander.
> >>>>> I think this has an issue if this SAS expander use a wide port
> >>>>> connecting a downstream SAS expander.
> >>>>> This is because if the PHY belongs to an existing port on this SAS
> >>>>> expander, the attached SAS address of this port must already be
> >>>>> present in the domain and it results in disabling that port.
> >>>>> I don’t think that is what we expect.
> >>>>>
> >>>>> In old release (4.x), at the end of this function, it would make
> >>>>> addition sas_ex_join_wide_port() call for any possibly PHYs that
> >>>>> could be added into the SAS port.
> >>>>> This will make subsequent PHYs (other than the first PHY of that
> >>>>> port) being marked to DISCOVERED so that this function would not
> >>>>> be invoked on those subsequent PHYs (in that port).
> >>>>> But potential question here is we didn’t configure the per-PHY
> >>>>> routing table for those PHYs.
> >>>>> As I don’t have such SAS expander on hand, I am not sure what’s
> >>>>> impact (maybe just performance/bandwidth impact).
> >>>>> But at least, it didn’t impact the functionality of that port.
> >>>>>
> >>>>> But in v5.3 or later release, that part of code was removed (in
> >>>>> the commit a1b6fb947f923).
> >>>>
> >>>> Jason, can you please check this?
> >>>
> >>> The removed code is only for races before we serialize the event
> >>> processing. All PHYs will still be scanned one by one and add to the
> >>> wide port if they have the same address. So are you encountering a
> >>> real issue? If
> >> so, can you share the full log?
> >>
> >> Yes. We did hit this issue when we upgrade Linux kernel from 4.19.236 to 5.14.21.
> >> Full logs attached.
> >>
> >>>
> >>> Thanks,
> >>> Jason
> >>>
> >>> 祝一切顺利！
> >>>
> >>>>
> >>>> Thanks!
> >>>>
> >>>>> And this caused this problem occurred (downstream port of that SAS
> >>>>> expander was disabled and all downstream SAS devices were removed
> >>>>> from the domain).
> >>>>>
> >>>>> Regards.
> >>>>> Eric Li
> >>>>>
> >>>>> SPE, DellEMC
> >>>>> 3/F KIC 1, 252# Songhu Road, YangPu District, SHANGHAI
> >>>>> +86-21-6036-4384
> >>>>>
> >>>>>
> >>>>> Internal Use - Confidential
> >>>>
> >>>> .