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

[Jason Yan <yanaijie@huawei.com>]

在 2026/6/23 10:43, Xingui Yang 写道:
> Introduce sas_dev_is_flutter() and sas_rediscover_ex_phy() to improve
> flutter and device replace detection during rediscovery.
> 
> sas_dev_is_flutter() adds validation for linkrate and sas_addr changes.
> When the SAS address changes, it restores phy->attached_sas_addr back to
> the original address before returning false, ensuring
> sas_unregister_devs_sas_addr() can properly match and unregister the old
> device via sas_phy_match_dev_addr().
> 
> The sas_addr check is ordered before the linkrate check to ensure the
> address restoration is not skipped when both change simultaneously.
> 
> Hold a kref on child_dev across the sas_ex_phy_discover() call to
> prevent use-after-free, since sas_ex_phy_discover() sends an SMP
> request which can sleep, during which the device could be freed by
> a concurrent removal path.
> 
> sas_rediscover_ex_phy() uses the async discovery pattern
> (sas_discover_event) instead of the synchronous sas_discover_new() to
> ensure proper ordering between device unregistration and rediscovery,
> avoiding sysfs_warn_dup() errors.
> 
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 89 +++++++++++++++++++++++++-----
>   1 file changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index cb9d3b748222..63d033e78985 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1966,6 +1966,78 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
>   	return false;
>   }
>   
> +static void sas_rediscover_ex_phy(struct domain_device *dev, int phy_id,
> +				  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_dev_is_flutter(struct domain_device *dev, int phy_id,
> +			       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 = NULL;
> +	char *action = "";
> +	int res;
> +
> +	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);
> +	if (!child_dev)
> +		goto out;
> +
> +	kref_get(&child_dev->kref);

This is not necessary so I think you can remove it as domain device will 
never release here sine we are in the discover workqueue process.
It's ture that sas_find_dev_by_rphy() is not perfect. It shall get a 
reference after lock ->dev_list_lock. But this will affect many existing 
users. We can do that in another patchset.

What do you think, John?

Jason
祝一切顺利

> +	res = sas_ex_phy_discover(dev, phy_id);
> +	if (res)
> +		goto out_put;
> +
> +	if (dev_is_sata(child_dev) &&
> +	    phy->attached_dev_type == SAS_SATA_PENDING) {
> +		action = ", needs recovery";
> +		goto out;
> +	}
> +
> +	if (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));
> +		/*
> +		 * Device unregistering relies on address matching. Restore
> +		 * attached_sas_addr back to the original address so that the old
> +		 * device can be unregistered later
> +		 */
> +		memcpy(phy->attached_sas_addr, child_dev->sas_addr, SAS_ADDR_SIZE);
> +		goto out_put;
> +	}
> +
> +	if (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);
> +		goto out_put;
> +	}
> +
> +out:
> +	if (child_dev)
> +		sas_put_device(child_dev);
> +	pr_debug("ex %016llx phy%02d broadcast flutter%s\n",
> +		 SAS_ADDR(dev->sas_addr), phy_id, action);
> +	return true;
> +out_put:
> +	sas_put_device(child_dev);
> +	return false;
> +}
> +
>   static int sas_rediscover_dev(struct domain_device *dev, int phy_id,
>   			      bool last, int sibling)
>   {
> @@ -2019,27 +2091,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_dev_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_ex_phy(dev, phy_id, last);
>   out_free_resp:
>   	kfree(disc_resp);
>   	return res;