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

Li, Eric (Honggang)
2025-06-10T14:43:33Z

Jason,

Sorry for bothering you.
Is this fix merged into kernel upstream?
If so, could you please let me know which Linux kernel version includes this fix?
Thanks.

Eric LI (Honggang)

> -----Original Message-----
> From: Li, Eric (Honggang)
> Sent: Tuesday, May 14, 2024 5:23 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: Saturday, May 11, 2024 11:41 AM
> > To: Li, Eric (Honggang) <Eric.H.Li@Dell.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
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On 2024/5/8 16:29, Li, Eric (Honggang) wrote:
> > >> -----Original Message-----
> > >> From: John Garry <john.g.garry@oracle.com>
> > >> Sent: Wednesday, May 8, 2024 3:48 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 08/05/2024 01:59, Li, Eric (Honggang) wrote:
> > >>>>> 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. >>
> > >>> OK. I will work on that and keep you updated.
> > >>
> > >> I expect a flow like this for scanning of the downstream expander:
> > >>
> > >> sas_discover_new(struct domain_device *dev [upstream expander], int
> > >> phy_id_a) -> sas_ex_discover_devices(single = -1) ->
> > >> sas_ex_discover_dev(phy_id_b) for each phy in @dev non-vacant and
> > >> non-discovered -> sas_ex_discover_expander( [downstream expander])
> > >> for first phy scanned which belongs to downstream expander.
> > >>
> > >> And following that we have continue to scan phys in
> > >> sas_ex_discover_devices(single = -1) ->
> > >> sas_ex_discover_dev(phy_id_b) ->
> > >> sas_ex_join_wide_port() -> for each non-vacant and non-discovered
> > >> phy in phy_id_b which matches that downstream expander.
> > >>
> > >> Can you see why this does not actually work/occur?
> > >>
> > >
> > > before calling sas_ex_join_wide_port(), sas_dev_present_in_domain()
> > > finds the attached_sas_address of PHY (phy_id_b) is already in the
> > > domain of that root
> > port, and then disable all PHYs to that downstream expander (in
> > sas_ex_disable_port(dev,
> > attached_sas_addr)) Therefore, I think we need to switch the order of
> > function call to
> > sas_ex_join_wide_port() and sas_dev_present_in_domain().
> >
> > Hi Eric,
> >
> > Can you test the following patch to see if it works?
> >
> > Author: Jason Yan <yanaijie@huawei.com>
> > Date: Sat May 11 11:33:35 2024 +0800
> >
> > scsi: libsas: Skip disable PHYs which can form wide ports
> >
> > Signed-off-by: Jason Yan <yanaijie@huawei.com>
> >
> > diff --git a/drivers/scsi/libsas/sas_expander.c
> > b/drivers/scsi/libsas/sas_expander.c
> > index f6e6db8b8aba..39a86857bc52 100644
> > --- a/drivers/scsi/libsas/sas_expander.c
> > +++ b/drivers/scsi/libsas/sas_expander.c
> > @@ -618,15 +618,17 @@ static void sas_ex_disable_port(struct
> > domain_device *dev, u8
> > *sas_addr)
> > }
> > }
> >
> > -static int sas_dev_present_in_domain(struct asd_sas_port *port,
> > +static int sas_dev_present_in_domain(struct domain_device *dev,
> > u8 *sas_addr)
> > {
> > - struct domain_device *dev;
> > + struct domain_device *tmp;
> >
> > if (SAS_ADDR(port->sas_addr) == SAS_ADDR(sas_addr))
> > return 1;
> > - list_for_each_entry(dev, &port->dev_list, dev_list_node) {
> > - if (SAS_ADDR(dev->sas_addr) == SAS_ADDR(sas_addr))
> > + list_for_each_entry(tmp, &dev->port->dev_list, dev_list_node) {
> > + if (tmp->parent == dev)
> > + continue;
> > + if (SAS_ADDR(tmp->sas_addr) == SAS_ADDR(sas_addr))
> > return 1;
> > }
> > return 0;
> > @@ -973,7 +975,7 @@ static int sas_ex_discover_dev(struct domain_device
> *dev, int phy_id)
> > return 0;
> > }
> >
> > - if (sas_dev_present_in_domain(dev->port, ex_phy->attached_sas_addr))
> > + if (sas_dev_present_in_domain(dev, ex_phy->attached_sas_addr))
> > sas_ex_disable_port(dev, ex_phy->attached_sas_addr);
> >
> > if (ex_phy->attached_dev_type == SAS_PHY_UNUSED) {
> >
> >
> >
>
> I am still waiting for feedback from our test team.
> From functionality, I think it should work as it skips sas_dev_present_in_domain
> check for the rest PHYs in the wide port.
> But from logic, I think it makes sense that checking wide port is prior to checking
> sas address duplication_in_domain.
> Just my two cents.