Re: [PATCH v4 2/2] scsi: libsas: Add linkrate and sas_addr change detection in rediscover

[yangxingui]

On 2026/5/29 22:09, John Garry wrote:
> > +static void sas_rediscover_phy(struct domain_device *dev, int phy_id,
>
> most of the expander phy symbols have _ex_phy ending

Ok, I will update in the next version.

> > +                   bool last)
> > +{
> > +    struct expander_device *ex = &dev->ex_dev;
> > +    struct ex_phy *phy = &ex->ex_phy[phy_id];
> > +
> > +    phy->phy_change_count = -1;
> > +    ex->ex_change_count = -1;
> > +    sas_unregister_devs_sas_addr(dev, phy_id, last);
> > +    sas_discover_event(dev->port, DISCE_REVALIDATE_DOMAIN);
> > +}
> > +
> > +static bool sas_is_flutter(struct domain_device *dev, int phy_id,
>
> sas_dev_is_flutter may be a better name

Ok.

> > +               u8 *sas_addr, enum sas_device_type type)
> > +{
> > +    struct expander_device *ex = &dev->ex_dev;
> > +    struct ex_phy *phy = &ex->ex_phy[phy_id];
> > +    struct domain_device *child_dev;
> > +    char *action = "";
> > +
> > +    if (SAS_ADDR(sas_addr) != SAS_ADDR(phy->attached_sas_addr) ||
> > +        !dev_type_flutter(type, phy->attached_dev_type))
> > +        return false;
> > +
> > +    child_dev = sas_ex_to_dev(dev, phy_id);
> > +
> > +    sas_ex_phy_discover(dev, phy_id);
>
> why not check the return code for error?

Hmm, I've considered this part as well. This section of code was 
directly extracted from the original code without any processing. I'll 
handle it in the next version as follow:

      res = sas_ex_phy_discover(dev, phy_id);
      if (res)
            return false;

If discover fails, we treat it as needing rediscovery rather than
ignoring the error.

> > +
> > +    if (child_dev && dev_is_sata(child_dev) &&
> > +        phy->attached_dev_type == SAS_SATA_PENDING) {
> > +        action = ", needs recovery";
> > +    } else if (child_dev && child_dev->linkrate != phy->linkrate) {
> > +        pr_info("ex %016llx phy%02d linkrate changed from %d to %d\n",
> > +            SAS_ADDR(dev->sas_addr), phy_id,
> > +            child_dev->linkrate, phy->linkrate);
> > +        return false;
> > +    } else if (child_dev &&
>
> Can you factor out the child_dev checks for all if/else legs?

Good idea, I will update in the next version.

> > +           SAS_ADDR(child_dev->sas_addr) != 
> > SAS_ADDR(phy->attached_sas_addr)) {
> > +        pr_info("ex %016llx phy%02d sas_addr changed from %016llx to 
> > %016llx\n",
> > +            SAS_ADDR(dev->sas_addr), phy_id,
> > +            SAS_ADDR(child_dev->sas_addr),
> > +            SAS_ADDR(phy->attached_sas_addr));
> > +        return false;
> > +    }
> > +
> > +    pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
> > +         SAS_ADDR(dev->sas_addr), phy_id, action);
> > +    return true;
> > +}
> > +
> >   static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
> >                     bool last, int sibling)
> >   {
> > @@ -2015,27 +2065,16 @@ static int sas_rediscover_dev(struct 
> > domain_device *dev, int phy_id,
> >           if (res == 0)
> >               sas_set_ex_phy(dev, phy_id, disc_resp);
> >           goto out_free_resp;
> > -    } else if (SAS_ADDR(sas_addr) == SAS_ADDR(phy->attached_sas_addr) &&
> > -           dev_type_flutter(type, phy->attached_dev_type)) {
> > -        struct domain_device *ata_dev = sas_ex_to_ata(dev, phy_id);
> > -        char *action = "";
> > -
> > -        sas_ex_phy_discover(dev, phy_id);
> > +    }
> > -        if (ata_dev && phy->attached_dev_type == SAS_SATA_PENDING)
> > -            action = ", needs recovery";
> > -        pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
> > -             SAS_ADDR(dev->sas_addr), phy_id, action);
> > +    if (sas_is_flutter(dev, phy_id, sas_addr, type))
> >           goto out_free_resp;
> > -    }
> >       /* we always have to delete the old device when we went here */
> >       pr_info("ex %016llx phy%02d replace %016llx\n",
> >           SAS_ADDR(dev->sas_addr), phy_id,
> >           SAS_ADDR(phy->attached_sas_addr));
> > -    sas_unregister_devs_sas_addr(dev, phy_id, last);
> > -
> > -    res = sas_discover_new(dev, phy_id);
> > +    sas_rediscover_phy(dev, phy_id, last);
> >   out_free_resp:
> >       kfree(disc_resp);
> >       return res;
>
> Can res still hold non-zero value from earlier?

Yes, I've considered this part as well, but it's always 
SMP_RESP_FUNC_ACC (0) when reaching the replace
branch. The code path to reach replace requires:

1. sas_get_phy_discover() returns SMP_RESP_FUNC_ACC (0)
2. SAS_ADDR(sas_addr) != 0 (otherwise goto out_free_resp)
3. res != -ECOMM (otherwise goto out_free_resp)
4. Not a flutter condition (otherwise goto out_free_resp)

So res is guaranteed to be 0 at the replace branch. I kept it unchanged
without explicitly setting res = 0.


Thanks.
Xingui