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

[PATCH v9 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 v8:
- In sas_dev_is_flutter(), call sas_ex_phy_discover() before
  sas_ex_to_dev() to ensure PHY state is always updated and to avoid
  use-after-free since the child device pointer is obtained after the
  sleeping SMP request completes, eliminating the need for kref
- Addressed Sashiko AI review [5][6] feedback on PHY discovery bypass
  and TOCTOU concerns

Changes from v7:
Addressed issues identified by Sashiko AI review [5][6]:
- In sas_dev_is_flutter(), reorder sas_addr check before linkrate check
  to ensure address restoration is not skipped when both change
  simultaneously, preventing device leak
- In sas_ex_to_dev(), add defensive NULL check for ex_dev to guard
  against callers passing a NULL device

Not addressed (pre-existing subsystem design):
- sas_find_dev_by_rphy() returns unreferenced pointer: subsystem-wide
  pattern used by 10+ call sites, should be a separate patch
- ex_phy->port TOCTOU: discovery path is serialized by disco_mutex,
  no race occurs in practice

Changes from v6:
- Add comment for restoring phy->attached_sas_addr to child_dev->sas_addr
- Optimize the conditional structure in sas_dev_is_flutter()

Changes from v5:
- In sas_addr change handling, restore phy->attached_sas_addr to
  child_dev->sas_addr before returning false, ensuring
  sas_unregister_devs_sas_addr() can properly match the device via
  sas_phy_match_dev_addr() for correct device unregistration

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/
[5] https://lore.kernel.org/linux-scsi/20260611062530.3B6651F00898@smtp.kernel.org/
[6] https://lore.kernel.org/linux-scsi/20260611062833.357031F00893@smtp.kernel.org/


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

-- 
2.43.0

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

[Xingui Yang]

Introduce sas_ex_to_dev() to return any device type attached to an
expander phy. The new helper is then used by sas_ex_to_ata() to reduce
code duplication.

Also add a defensive NULL check for ex_dev to guard against callers
passing a NULL device.

Signed-off-by: Xingui Yang <yangxingui@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 19 ++++++++++++++-----
 drivers/scsi/libsas/sas_internal.h |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index f471ab464a78..fc6d8f3c9dca 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -345,13 +345,16 @@ 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)
+/* return the domain device attached to an expander phy */
+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 ex_phy *ex_phy;
 	struct sas_rphy *rphy;
 
+	if (!ex_dev)
+		return NULL;
+
+	ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
 	if (!ex_phy->port)
 		return NULL;
 
@@ -359,7 +362,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 v9 2/2] scsi: libsas: Add linkrate and sas_addr change detection in rediscover

[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() calls sas_ex_phy_discover() before looking up the
child device, which ensures the PHY state is always updated and avoids
a use-after-free since the child device pointer is obtained after the
sleeping SMP request completes.

It 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.

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>
Suggested-by: John Garry <john.g.garry@oracle.com>
---
 drivers/scsi/libsas/sas_expander.c | 83 +++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index fc6d8f3c9dca..e27953de2b4e 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1967,6 +1967,72 @@ 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;
+
+	res = sas_ex_phy_discover(dev, phy_id);
+	if (res)
+		return false;
+
+	child_dev = sas_ex_to_dev(dev, phy_id);
+	if (!child_dev)
+		goto out;
+
+	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);
+		return false;
+	}
+
+	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;
+	}
+
+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)
 {
@@ -2020,27 +2086,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 v9 2/2] scsi: libsas: Add linkrate and sas_addr change detection in rediscover

[yangxingui]

Kindly ping for review...

On 2026/6/24 14:32, Xingui Yang wrote:
> Introduce sas_dev_is_flutter() and sas_rediscover_ex_phy() to improve
> flutter and device replace detection during rediscovery.
> 
> sas_dev_is_flutter() calls sas_ex_phy_discover() before looking up the
> child device, which ensures the PHY state is always updated and avoids
> a use-after-free since the child device pointer is obtained after the
> sleeping SMP request completes.
> 
> It 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.
> 
> 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>
> Suggested-by: John Garry <john.g.garry@oracle.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 83 +++++++++++++++++++++++++-----
>   1 file changed, 69 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index fc6d8f3c9dca..e27953de2b4e 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -1967,6 +1967,72 @@ 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;
> +
> +	res = sas_ex_phy_discover(dev, phy_id);
> +	if (res)
> +		return false;
> +
> +	child_dev = sas_ex_to_dev(dev, phy_id);
> +	if (!child_dev)
> +		goto out;
> +
> +	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);
> +		return false;
> +	}
> +
> +	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;
> +	}
> +
> +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)
>   {
> @@ -2020,27 +2086,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;
> 

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

[yangxingui]

Hi, John

Kindly ping for review...

On 2026/6/24 14:32, Xingui Yang wrote:
> 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 v8:
> - In sas_dev_is_flutter(), call sas_ex_phy_discover() before
>    sas_ex_to_dev() to ensure PHY state is always updated and to avoid
>    use-after-free since the child device pointer is obtained after the
>    sleeping SMP request completes, eliminating the need for kref
> - Addressed Sashiko AI review [5][6] feedback on PHY discovery bypass
>    and TOCTOU concerns
> 
> Changes from v7:
> Addressed issues identified by Sashiko AI review [5][6]:
> - In sas_dev_is_flutter(), reorder sas_addr check before linkrate check
>    to ensure address restoration is not skipped when both change
>    simultaneously, preventing device leak
> - In sas_ex_to_dev(), add defensive NULL check for ex_dev to guard
>    against callers passing a NULL device
> 
> Not addressed (pre-existing subsystem design):
> - sas_find_dev_by_rphy() returns unreferenced pointer: subsystem-wide
>    pattern used by 10+ call sites, should be a separate patch
> - ex_phy->port TOCTOU: discovery path is serialized by disco_mutex,
>    no race occurs in practice
> 
> Changes from v6:
> - Add comment for restoring phy->attached_sas_addr to child_dev->sas_addr
> - Optimize the conditional structure in sas_dev_is_flutter()
> 
> Changes from v5:
> - In sas_addr change handling, restore phy->attached_sas_addr to
>    child_dev->sas_addr before returning false, ensuring
>    sas_unregister_devs_sas_addr() can properly match the device via
>    sas_phy_match_dev_addr() for correct device unregistration
> 
> 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/
> [5] https://lore.kernel.org/linux-scsi/20260611062530.3B6651F00898@smtp.kernel.org/
> [6] https://lore.kernel.org/linux-scsi/20260611062833.357031F00893@smtp.kernel.org/
> 
> 
> 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 | 102 +++++++++++++++++++++++------
>   drivers/scsi/libsas/sas_internal.h |   1 +
>   2 files changed, 84 insertions(+), 19 deletions(-)
> 

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

[John Garry]

On 24/06/2026 07:32, Xingui Yang wrote:
> Introduce sas_ex_to_dev() to return any device type attached to an
> expander phy. The new helper is then used by sas_ex_to_ata() to reduce
> code duplication.
> 
> Also add a defensive NULL check for ex_dev to guard against callers
> passing a NULL device.
> 

Why would someone want to lookup a device attached to an expander device 
which is NULL? If this is possible later, then mention it.

> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
>   drivers/scsi/libsas/sas_expander.c | 19 ++++++++++++++-----
>   drivers/scsi/libsas/sas_internal.h |  1 +
>   2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f471ab464a78..fc6d8f3c9dca 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -345,13 +345,16 @@ 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)
> +/* return the domain device attached to an expander phy */
> +struct domain_device *sas_ex_to_dev(struct domain_device *ex_dev, int phy_id)
>   {

sas_ex_phy_to_dev() seems a better name, as sas_ex_to_dev() is quite 
vague. I know that it matches sas_ex_to_ata(), but it is not a good 
pattern to match.

> -	struct ex_phy *ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
> -	struct domain_device *dev;
> +	struct ex_phy *ex_phy;
>   	struct sas_rphy *rphy;
>   
> +	if (!ex_dev)
> +		return NULL;
> +
> +	ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
>   	if (!ex_phy->port)
>   		return NULL;
>   
> @@ -359,7 +362,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,

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

[yangxingui]

Hi John,

Thanks for the review.

On 2026/8/10 23:51, John Garry wrote:
> On 24/06/2026 07:32, Xingui Yang wrote:
>> Introduce sas_ex_to_dev() to return any device type attached to an
>> expander phy. The new helper is then used by sas_ex_to_ata() to reduce
>> code duplication.
>>
>> Also add a defensive NULL check for ex_dev to guard against callers
>> passing a NULL device.
>>
> 
> Why would someone want to lookup a device attached to an expander device 
> which is NULL? If this is possible later, then mention it.

No current caller passes a NULL ex_dev. The check was added in v7 based 
on an automated Sashiko AI review suggestion, which may contain some 
noise. Removed in v10.

>> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
>> ---
>>   drivers/scsi/libsas/sas_expander.c | 19 ++++++++++++++-----
>>   drivers/scsi/libsas/sas_internal.h |  1 +
>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index f471ab464a78..fc6d8f3c9dca 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -345,13 +345,16 @@ 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)
>> +/* return the domain device attached to an expander phy */
>> +struct domain_device *sas_ex_to_dev(struct domain_device *ex_dev, int 
>> phy_id)
>>   {
> 
> sas_ex_phy_to_dev() seems a better name, as sas_ex_to_dev() is quite 
> vague. I know that it matches sas_ex_to_ata(), but it is not a good 
> pattern to match.

Ok, Renamed to sas_ex_phy_to_dev() for consistency with 
sas_ex_phy_discover() and other phy-related helpers.


Thanks.
Xingui