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

[PATCH v5 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 v4:
- Rename sas_rediscover_phy to sas_rediscover_ex_phy for consistency
  with expander phy symbol naming convention
- Rename sas_is_flutter to sas_dev_is_flutter per John's suggestion
- Check return value of sas_ex_phy_discover() for errors
- Factor out child_dev checks to improve code clarity

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 | 83 +++++++++++++++++++++++-------
 drivers/scsi/libsas/sas_internal.h |  1 +
 2 files changed, 66 insertions(+), 18 deletions(-)

-- 
2.43.0

[PATCH v5 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 v5 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_dev_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_ex_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 | 71 ++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index f55ae9a979cd..7246e41aee12 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1962,6 +1962,60 @@ 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;
+	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;
+
+	res = sas_ex_phy_discover(dev, phy_id);
+	if (res)
+		return false;
+
+	if (dev_is_sata(child_dev) &&
+	    phy->attached_dev_type == SAS_SATA_PENDING) {
+		action = ", needs recovery";
+	} else 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);
+		return false;
+	} else 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));
+		return false;
+	}
+out:
+	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 +2069,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;
-- 
2.43.0

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

[John Garry]

On 30/05/2026 03:49, 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_dev_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_ex_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>

This looks ok, so:

Reviewed-by: John Garry <john.g.garry@oracle.com>

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

[yangxingui]

On 2026/6/3 0:30, John Garry wrote:
> On 30/05/2026 03:49, 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_dev_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_ex_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>
> 
> This looks ok, so:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>

Hi, John

Thank you for your review!

After further analysis, I found a small issue in the sas_addr change
handling that needs a minor fix:

When sas_addr change is detected in sas_dev_is_flutter(), after
sas_ex_phy_discover() updates phy->attached_sas_addr to the new address,
subsequent sas_unregister_devs_sas_addr() cannot properly match the
device because sas_phy_match_dev_addr() compares phy->attached_sas_addr
with child_dev->sas_addr, which would mismatch.

So I added a memcpy() to restore phy->attached_sas_addr to
child_dev->sas_addr before returning false, ensuring proper device
unregistration:

     memcpy(phy->attached_sas_addr, child_dev->sas_addr,
            SAS_ADDR_SIZE);

This change is included in v6. Would you mind taking another
look when you have time?

Thanks,
Xingui
.