[PATCH v4 0/2] libsas: rediscover improvements for linkrate/sas_addr changes

[PATCH v4 0/2] libsas: rediscover improvements for linkrate/sas_addr changes

[Xingui Yang]

When a device attached to an expander phy experiences a linkrate change
(e.g., due to cable reconnection or negotiation), the current code in
sas_rediscover_dev() treats it as "broadcast flutter" and takes no action
if the SAS address and device type remain unchanged.

This series is based on John Garry's suggestion [1] to check the linkrate
and mark the device as gone and rediscover when flutter occurs, replacing
the previous v2 patch series that used lldd callbacks.

The previous v2 approach added lldd_dev_info_update callback which John
commented as "seem fragile and too specialized" [2]. This series adopts
a simpler approach that directly checks linkrate/sas_addr changes in
sas_rediscover_dev() and triggers rediscovery using libsas's standard
async discovery pattern.

This aligns with Jason Yan's earlier work [3] which was verified to
solve the linkrate change issue.

Additionally, per the discussion in v3 [4], the existing replace code
path also suffers from the same sysfs duplication issue: 
sas_unregister_devs_sas_addr() only marks the device as gone, but the
actual sysfs cleanup happens later in sas_destruct_devices(). Calling
sas_discover_new() immediately after unregister causes sysfs_warn_dup()
errors. This series also optimizes the replace path to use the async
pattern, ensuring proper ordering for both flutter and replace cases.

Changes from v3:
- Also optimize the replace code path to use async discovery pattern
- Introduce sas_is_flutter() and sas_rediscover_phy() helpers
  to encapsulate the flutter handling logic and avoid function bloat
- Fix replace code path sysfs duplication issue

Changes from v2:
- Drop lldd_dev_info_update callback approach per John Garry's suggestion
- Drop hisi_sas specific changes (no longer needed without callback)
- Use libsas's async discovery pattern for rediscovery
- Add sas_addr change detection alongside linkrate change

Changes from v1:
- Split into three patches

[1] https://lore.kernel.org/linux-scsi/c4e4c99f-a13c-4e28-8650-48be1f96d7cf@oracle.com/
[2] https://lore.kernel.org/linux-scsi/28bd9d5b-f597-0aae-5340-bd951b2083aa@huawei.com/
[3] https://lore.kernel.org/linux-scsi/20190130082412.9357-6-yanaijie@huawei.com/
[4] https://lore.kernel.org/linux-scsi/b99cd59f-b986-432e-aaf1-3b757e1c4c34@oracle.com/

Xingui Yang (2):
  scsi: libsas: refactor sas_ex_to_ata() using new helper
    sas_ex_to_dev()
  scsi: libsas: Add linkrate and sas_addr change detection in rediscover

 drivers/scsi/libsas/sas_expander.c | 79 +++++++++++++++++++++++-------
 drivers/scsi/libsas/sas_internal.h |  1 +
 2 files changed, 62 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH v4 1/2] scsi: libsas: refactor sas_ex_to_ata() using new helper sas_ex_to_dev()

[Xingui Yang]

The sas_ex_to_ata() function checks for an attached ATA device on an
expander phy. Refactor it to use a new helper function sas_ex_to_dev()
which returns any device type attached to an expander phy, improving code
reuse and allowing other code paths to find attached devices regardless
of type.

No functional changes intended.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
Reviewed-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 12 ++++++++----
 drivers/scsi/libsas/sas_internal.h |  1 +
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index f471ab464a78..f55ae9a979cd 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -345,11 +345,9 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
 		 SAS_ADDR(phy->attached_sas_addr), type);
 }
 
-/* check if we have an existing attached ata device on this expander phy */
-struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id)
+struct domain_device *sas_ex_to_dev(struct domain_device *ex_dev, int phy_id)
 {
 	struct ex_phy *ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
-	struct domain_device *dev;
 	struct sas_rphy *rphy;
 
 	if (!ex_phy->port)
@@ -359,7 +357,13 @@ struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id)
 	if (!rphy)
 		return NULL;
 
-	dev = sas_find_dev_by_rphy(rphy);
+	return sas_find_dev_by_rphy(rphy);
+}
+
+/* check if we have an existing attached ata device on this expander phy */
+struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id)
+{
+	struct domain_device *dev = sas_ex_to_dev(ex_dev, phy_id);
 
 	if (dev && dev_is_sata(dev))
 		return dev;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 7dce0f587149..350a70484bde 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -91,6 +91,7 @@ int sas_smp_get_phy_events(struct sas_phy *phy);
 
 void sas_device_set_phy(struct domain_device *dev, struct sas_port *port);
 struct domain_device *sas_find_dev_by_rphy(struct sas_rphy *rphy);
+struct domain_device *sas_ex_to_dev(struct domain_device *ex_dev, int phy_id);
 struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id);
 int sas_ex_phy_discover(struct domain_device *dev, int single);
 int sas_get_report_phy_sata(struct domain_device *dev, int phy_id,
-- 
2.43.0

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

[Xingui Yang]

In sas_rediscover_dev(), when detecting a "flutter" condition (same SAS
address and compatible device type), the code assumes the device remains
unchanged and only handles SATA pending state recovery. However, this
approach misses two important scenarios:

First, the flutter detection only compares SAS address and device type,
ignoring potential linkrate changes that may have already occurred.

Second, after sas_ex_phy_discover() re-queries the expander phy, both
linkrate and attached SAS address may be updated. The current code does
not validate these changes against the existing child device.

Additionally, the replace code path (different SAS address detected)
has a sysfs duplication issue: sas_unregister_devs_sas_addr() only marks
the device as gone, but the actual sysfs cleanup happens later in
sas_destruct_devices(). Calling sas_discover_new() immediately after
unregister causes sysfs_warn_dup() errors.

Introduce sas_is_flutter() to check whether it is a true flutter with
validation for linkrate and sas_addr changes. It returns true for normal
flutter and false when changes are detected requiring rediscovery.

Introduce sas_rediscover_phy() to handle async rediscovery for both
flutter and replace cases. When invoked:
- Set phy_change_count and ex_change_count to -1 to force revalidation
- Unregister the device via sas_unregister_devs_sas_addr()
- Queue DISCE_REVALIDATE_DOMAIN event

The old device sysfs is cleaned up by sas_destruct_devices() at the end
of current revalidation work. The new event triggers discovery via
sas_discover_new() since attached_sas_addr is cleared, avoiding the
sysfs duplication issue.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
Suggested-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/libsas/sas_expander.c | 67 +++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index f55ae9a979cd..4e8e1b339889 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1962,6 +1962,56 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
 	return false;
 }
 
+static void sas_rediscover_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_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;
+	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);
+
+	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 &&
+		   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;
-- 
2.43.0

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

[John Garry]

On 26/05/2026 02:54, Xingui Yang wrote:
> In sas_rediscover_dev(), when detecting a "flutter" condition (same SAS
> address and compatible device type), the code assumes the device remains
> unchanged and only handles SATA pending state recovery. However, this
> approach misses two important scenarios:
> 
> First, the flutter detection only compares SAS address and device type,
> ignoring potential linkrate changes that may have already occurred.
> 
> Second, after sas_ex_phy_discover() re-queries the expander phy, both
> linkrate and attached SAS address may be updated. The current code does
> not validate these changes against the existing child device.
> 
> Additionally, the replace code path (different SAS address detected)
> has a sysfs duplication issue: sas_unregister_devs_sas_addr() only marks
> the device as gone, but the actual sysfs cleanup happens later in
> sas_destruct_devices(). Calling sas_discover_new() immediately after
> unregister causes sysfs_warn_dup() errors.
> 
> Introduce sas_is_flutter() to check whether it is a true flutter with
> validation for linkrate and sas_addr changes. It returns true for normal
> flutter and false when changes are detected requiring rediscovery.
> 
> Introduce sas_rediscover_phy() to handle async rediscovery for both
> flutter and replace cases. When invoked:
> - Set phy_change_count and ex_change_count to -1 to force revalidation
> - Unregister the device via sas_unregister_devs_sas_addr()
> - Queue DISCE_REVALIDATE_DOMAIN event
> 
> The old device sysfs is cleaned up by sas_destruct_devices() at the end
> of current revalidation work. The new event triggers discovery via
> sas_discover_new() since attached_sas_addr is cleared, avoiding the
> sysfs duplication issue.
> 
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> Suggested-by: John Garry <john.g.garry@oracle.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 67 +++++++++++++++++++++++-------
>   1 file changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f55ae9a979cd..4e8e1b339889 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1962,6 +1962,56 @@ static bool dev_type_flutter(enum sas_device_type new, enum sas_device_type old)
>   	return false;
>   }
>   
> +static void sas_rediscover_phy(struct domain_device *dev, int phy_id,

most of the expander phy symbols have _ex_phy ending

> +			       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

> +			   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?

> +
> +	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?

> +		   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?

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