[PATCH v4 0/8] scsi: libsas: sas address comparison refactor

[PATCH v4 0/8] scsi: libsas: sas address comparison refactor

[Jason Yan]

Sas address conversion and comparison is widely used in libsas and
drivers. However they are all opencoded and to avoid the line spill over
80 columns, are mostly split into multi-lines.

To make the code easier to read, introduce some helpers with clearer
semantics and replace the opencoded segments with them.

v3->v4:
  Fix comparison typo.
  Fix test condition error in sas_check_parent_topology() of patch #6.

v2->v3:
  Rename sas_phy_addr_same() to sas_phy_addr_match().
  Rearrange patches, move patch #6 to #1 and directly use the helper
  	sas_phy_match_dev_addr() in sas_find_attached_phy().
  Add some review tags from Jack Wang.

v1->v2:
  First factor out sas_find_attached_phy() and replace LLDDs's code
  	with it.
  Remove three too simple helpers.
  Rename the helpers with 'sas_' prefix.

Jason Yan (8):
  scsi: libsas: introduce sas address comparison helpers
  scsi: libsas: introduce sas_find_attached_phy() helper
  scsi: pm8001: use sas_find_attached_phy() instead of open coded
  scsi: mvsas: use sas_find_attached_phy() instead of open coded
  scsi: hisi_sas: use sas_find_attathed_phy() instead of open coded
  scsi: libsas: use sas_phy_match_dev_addr() instead of open coded
  scsi: libsas: use sas_phy_addr_match() instead of open coded
  scsi: libsas: use sas_phy_match_port_addr() instead of open coded

 drivers/scsi/hisi_sas/hisi_sas_main.c | 12 ++------
 drivers/scsi/libsas/sas_expander.c    | 40 ++++++++++++++++-----------
 drivers/scsi/libsas/sas_internal.h    | 17 ++++++++++++
 drivers/scsi/mvsas/mv_sas.c           | 15 +++-------
 drivers/scsi/pm8001/pm8001_sas.c      | 16 ++++-------
 include/scsi/libsas.h                 |  2 ++
 6 files changed, 54 insertions(+), 48 deletions(-)

-- 
2.31.1

[PATCH v4 1/8] scsi: libsas: introduce sas address comparison helpers

[Jason Yan]

Sas address comparison is widely used in libsas. However they are all
opencoded and to avoid the line spill over 80 columns, are mostly split
into multi-lines. Introduce some helpers to prepare some refactor.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/libsas/sas_internal.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 8d0ad3abc7b5..3384429b7eb0 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -111,6 +111,23 @@ static inline void sas_smp_host_handler(struct bsg_job *job,
 }
 #endif
 
+static inline bool sas_phy_match_dev_addr(struct domain_device *dev,
+					 struct ex_phy *phy)
+{
+	return SAS_ADDR(dev->sas_addr) == SAS_ADDR(phy->attached_sas_addr);
+}
+
+static inline bool sas_phy_match_port_addr(struct asd_sas_port *port,
+					   struct ex_phy *phy)
+{
+	return SAS_ADDR(port->sas_addr) == SAS_ADDR(phy->attached_sas_addr);
+}
+
+static inline bool sas_phy_addr_match(struct ex_phy *p1, struct ex_phy *p2)
+{
+	return  SAS_ADDR(p1->attached_sas_addr) == SAS_ADDR(p2->attached_sas_addr);
+}
+
 static inline void sas_fail_probe(struct domain_device *dev, const char *func, int err)
 {
 	pr_warn("%s: for %s device %016llx returned %d\n",
-- 
2.31.1

[PATCH v4 2/8] scsi: libsas: introduce sas_find_attached_phy() helper

[Jason Yan]

LLDDs are implementing their own attached phy finding code repeatedly.
Factor it out to libsas.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/libsas/sas_expander.c | 16 ++++++++++++++++
 include/scsi/libsas.h              |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index fa2209080cc2..df5a64ad902f 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -2107,6 +2107,22 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev)
 	return res;
 }
 
+int sas_find_attached_phy(struct expander_device *ex_dev,
+			  struct domain_device *dev)
+{
+	struct ex_phy *phy;
+	int phy_id;
+
+	for (phy_id = 0; phy_id < ex_dev->num_phys; phy_id++) {
+		phy = &ex_dev->ex_phy[phy_id];
+		if (sas_phy_match_dev_addr(dev, phy))
+			return phy_id;
+	}
+
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(sas_find_attached_phy);
+
 void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
 		struct sas_rphy *rphy)
 {
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2dbead74a2af..75faf2308eae 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -750,6 +750,8 @@ int sas_clear_task_set(struct domain_device *dev, u8 *lun);
 int sas_lu_reset(struct domain_device *dev, u8 *lun);
 int sas_query_task(struct sas_task *task, u16 tag);
 int sas_abort_task(struct sas_task *task, u16 tag);
+int sas_find_attached_phy(struct expander_device *ex_dev,
+			  struct domain_device *dev);
 
 void sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
 			   gfp_t gfp_flags);
-- 
2.31.1

[PATCH v4 3/8] scsi: pm8001: use sas_find_attached_phy() instead of open coded

[Jason Yan]

The attached phy finding is open coded. Now we can replace it with
sas_find_attached_phy().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/pm8001/pm8001_sas.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 8e3f2f9ddaac..d15a824bcea6 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -645,22 +645,16 @@ static int pm8001_dev_found_notify(struct domain_device *dev)
 	pm8001_device->dcompletion = &completion;
 	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
 		int phy_id;
-		struct ex_phy *phy;
-		for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
-		phy_id++) {
-			phy = &parent_dev->ex_dev.ex_phy[phy_id];
-			if (SAS_ADDR(phy->attached_sas_addr)
-				== SAS_ADDR(dev->sas_addr)) {
-				pm8001_device->attached_phy = phy_id;
-				break;
-			}
-		}
-		if (phy_id == parent_dev->ex_dev.num_phys) {
+
+		phy_id = sas_find_attached_phy(&parent_dev->ex_dev, dev);
+		if (phy_id == -ENODEV) {
 			pm8001_dbg(pm8001_ha, FAIL,
 				   "Error: no attached dev:%016llx at ex:%016llx.\n",
 				   SAS_ADDR(dev->sas_addr),
 				   SAS_ADDR(parent_dev->sas_addr));
 			res = -1;
+		} else {
+			pm8001_device->attached_phy = phy_id;
 		}
 	} else {
 		if (dev->dev_type == SAS_SATA_DEV) {
-- 
2.31.1

[PATCH v4 4/8] scsi: mvsas: use sas_find_attached_phy() instead of open coded

[Jason Yan]

The attached phy finding is open coded. Now we can replace it with
sas_find_attached_phy().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mvsas/mv_sas.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index a6867dae0e7c..f6576ffabe9f 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -1190,23 +1190,16 @@ static int mvs_dev_found_notify(struct domain_device *dev, int lock)
 	mvi_device->sas_device = dev;
 	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
 		int phy_id;
-		u8 phy_num = parent_dev->ex_dev.num_phys;
-		struct ex_phy *phy;
-		for (phy_id = 0; phy_id < phy_num; phy_id++) {
-			phy = &parent_dev->ex_dev.ex_phy[phy_id];
-			if (SAS_ADDR(phy->attached_sas_addr) ==
-				SAS_ADDR(dev->sas_addr)) {
-				mvi_device->attached_phy = phy_id;
-				break;
-			}
-		}
 
-		if (phy_id == phy_num) {
+		phy_id = sas_find_attached_phy(&parent_dev->ex_dev, dev);
+		if (phy_id == -ENODEV) {
 			mv_printk("Error: no attached dev:%016llx"
 				"at ex:%016llx.\n",
 				SAS_ADDR(dev->sas_addr),
 				SAS_ADDR(parent_dev->sas_addr));
 			res = -1;
+		} else {
+			mvi_device->attached_phy = phy_id;
 		}
 	}
 
-- 
2.31.1

[PATCH v4 5/8] scsi: hisi_sas: use sas_find_attathed_phy() instead of open coded

[Jason Yan]

The attached phy finding is open coded. Now we can replace it with
sas_find_attached_phy().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 33af5b8dede2..995ccb13fb9d 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -792,17 +792,9 @@ static int hisi_sas_dev_found(struct domain_device *device)
 
 	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
 		int phy_no;
-		u8 phy_num = parent_dev->ex_dev.num_phys;
-		struct ex_phy *phy;
 
-		for (phy_no = 0; phy_no < phy_num; phy_no++) {
-			phy = &parent_dev->ex_dev.ex_phy[phy_no];
-			if (SAS_ADDR(phy->attached_sas_addr) ==
-				SAS_ADDR(device->sas_addr))
-				break;
-		}
-
-		if (phy_no == phy_num) {
+		phy_no = sas_find_attached_phy(&parent_dev->ex_dev, device);
+		if (phy_no == -ENODEV) {
 			dev_info(dev, "dev found: no attached "
 				 "dev:%016llx at ex:%016llx\n",
 				 SAS_ADDR(device->sas_addr),
-- 
2.31.1

[PATCH v4 6/8] scsi: libsas: use sas_phy_match_dev_addr() instead of open coded

[Jason Yan]

The sas address comparison of domain device and expander phy is open
coded. Now we can replace it with sas_phy_match_dev_addr().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index df5a64ad902f..06efdfc11d2e 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -738,9 +738,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
 		    phy->phy_state == PHY_NOT_PRESENT)
 			continue;
 
-		if (SAS_ADDR(phy->attached_sas_addr) ==
-		    SAS_ADDR(child->sas_addr)) {
-
+		if (sas_phy_match_dev_addr(child, phy)) {
 			child->min_linkrate = min(parent->min_linkrate,
 						  phy->linkrate);
 			child->max_linkrate = max(parent->max_linkrate,
@@ -1012,8 +1010,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 		sas_add_parent_port(dev, phy_id);
 		return 0;
 	}
-	if (dev->parent && (SAS_ADDR(ex_phy->attached_sas_addr) ==
-			    SAS_ADDR(dev->parent->sas_addr))) {
+	if (dev->parent && sas_phy_match_dev_addr(dev->parent, ex_phy)) {
 		sas_add_parent_port(dev, phy_id);
 		if (ex_phy->routing_attr == TABLE_ROUTING)
 			sas_configure_phy(dev, phy_id, dev->port->sas_addr, 1);
@@ -1312,7 +1309,7 @@ static int sas_check_parent_topology(struct domain_device *child)
 		    parent_phy->phy_state == PHY_NOT_PRESENT)
 			continue;
 
-		if (SAS_ADDR(parent_phy->attached_sas_addr) != SAS_ADDR(child->sas_addr))
+		if (!sas_phy_match_dev_addr(child, parent_phy))
 			continue;
 
 		child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id];
@@ -1522,8 +1519,7 @@ static int sas_configure_parent(struct domain_device *parent,
 		struct ex_phy *phy = &ex_parent->ex_phy[i];
 
 		if ((phy->routing_attr == TABLE_ROUTING) &&
-		    (SAS_ADDR(phy->attached_sas_addr) ==
-		     SAS_ADDR(child->sas_addr))) {
+		    sas_phy_match_dev_addr(child, phy)) {
 			res = sas_configure_phy(parent, i, sas_addr, include);
 			if (res)
 				return res;
@@ -1858,8 +1854,7 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent,
 	if (last) {
 		list_for_each_entry_safe(child, n,
 			&ex_dev->children, siblings) {
-			if (SAS_ADDR(child->sas_addr) ==
-			    SAS_ADDR(phy->attached_sas_addr)) {
+			if (sas_phy_match_dev_addr(child, phy)) {
 				set_bit(SAS_DEV_GONE, &child->state);
 				if (dev_is_expander(child->dev_type))
 					sas_unregister_ex_tree(parent->port, child);
@@ -1941,8 +1936,7 @@ static int sas_discover_new(struct domain_device *dev, int phy_id)
 	if (res)
 		return res;
 	list_for_each_entry(child, &dev->ex_dev.children, siblings) {
-		if (SAS_ADDR(child->sas_addr) ==
-		    SAS_ADDR(ex_phy->attached_sas_addr)) {
+		if (sas_phy_match_dev_addr(child, ex_phy)) {
 			if (dev_is_expander(child->dev_type))
 				res = sas_discover_bfs_by_root(child);
 			break;
-- 
2.31.1

[PATCH v4 7/8] scsi: libsas: use sas_phy_addr_match() instead of open coded

[Jason Yan]

The sas address comparison of expander phys is open coded. Now we can
replace it with sas_phy_addr_match().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/libsas/sas_expander.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 06efdfc11d2e..8d92f9940d51 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -2058,8 +2058,7 @@ static int sas_rediscover(struct domain_device *dev, const int phy_id)
 
 			if (i == phy_id)
 				continue;
-			if (SAS_ADDR(phy->attached_sas_addr) ==
-			    SAS_ADDR(changed_phy->attached_sas_addr)) {
+			if (sas_phy_addr_match(phy, changed_phy)) {
 				last = false;
 				break;
 			}
-- 
2.31.1

[PATCH v4 8/8] scsi: libsas: use sas_phy_match_port_addr() instead of open coded

[Jason Yan]

The sas address comparison of asd_sas_port and expander phy is open
coded. Now we can replace it with sas_phy_match_port_addr().

Signed-off-by: Jason Yan <yanaijie@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/libsas/sas_expander.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 8d92f9940d51..41f1578681cb 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -1005,8 +1005,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
 	}
 
 	/* Parent and domain coherency */
-	if (!dev->parent && (SAS_ADDR(ex_phy->attached_sas_addr) ==
-			     SAS_ADDR(dev->port->sas_addr))) {
+	if (!dev->parent && sas_phy_match_port_addr(dev->port, ex_phy)) {
 		sas_add_parent_port(dev, phy_id);
 		return 0;
 	}
-- 
2.31.1

Re: [PATCH v4 6/8] scsi: libsas: use sas_phy_match_dev_addr() instead of open coded

[Damien Le Moal]

On 9/27/22 12:26, Jason Yan wrote:
> The sas address comparison of domain device and expander phy is open
> coded. Now we can replace it with sas_phy_match_dev_addr().
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>

Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

> ---
>  drivers/scsi/libsas/sas_expander.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index df5a64ad902f..06efdfc11d2e 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -738,9 +738,7 @@ static void sas_ex_get_linkrate(struct domain_device *parent,
>  		    phy->phy_state == PHY_NOT_PRESENT)
>  			continue;
>  
> -		if (SAS_ADDR(phy->attached_sas_addr) ==
> -		    SAS_ADDR(child->sas_addr)) {
> -
> +		if (sas_phy_match_dev_addr(child, phy)) {
>  			child->min_linkrate = min(parent->min_linkrate,
>  						  phy->linkrate);
>  			child->max_linkrate = max(parent->max_linkrate,
> @@ -1012,8 +1010,7 @@ static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
>  		sas_add_parent_port(dev, phy_id);
>  		return 0;
>  	}
> -	if (dev->parent && (SAS_ADDR(ex_phy->attached_sas_addr) ==
> -			    SAS_ADDR(dev->parent->sas_addr))) {
> +	if (dev->parent && sas_phy_match_dev_addr(dev->parent, ex_phy)) {
>  		sas_add_parent_port(dev, phy_id);
>  		if (ex_phy->routing_attr == TABLE_ROUTING)
>  			sas_configure_phy(dev, phy_id, dev->port->sas_addr, 1);
> @@ -1312,7 +1309,7 @@ static int sas_check_parent_topology(struct domain_device *child)
>  		    parent_phy->phy_state == PHY_NOT_PRESENT)
>  			continue;
>  
> -		if (SAS_ADDR(parent_phy->attached_sas_addr) != SAS_ADDR(child->sas_addr))
> +		if (!sas_phy_match_dev_addr(child, parent_phy))
>  			continue;
>  
>  		child_phy = &child_ex->ex_phy[parent_phy->attached_phy_id];
> @@ -1522,8 +1519,7 @@ static int sas_configure_parent(struct domain_device *parent,
>  		struct ex_phy *phy = &ex_parent->ex_phy[i];
>  
>  		if ((phy->routing_attr == TABLE_ROUTING) &&
> -		    (SAS_ADDR(phy->attached_sas_addr) ==
> -		     SAS_ADDR(child->sas_addr))) {
> +		    sas_phy_match_dev_addr(child, phy)) {
>  			res = sas_configure_phy(parent, i, sas_addr, include);
>  			if (res)
>  				return res;
> @@ -1858,8 +1854,7 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent,
>  	if (last) {
>  		list_for_each_entry_safe(child, n,
>  			&ex_dev->children, siblings) {
> -			if (SAS_ADDR(child->sas_addr) ==
> -			    SAS_ADDR(phy->attached_sas_addr)) {
> +			if (sas_phy_match_dev_addr(child, phy)) {
>  				set_bit(SAS_DEV_GONE, &child->state);
>  				if (dev_is_expander(child->dev_type))
>  					sas_unregister_ex_tree(parent->port, child);
> @@ -1941,8 +1936,7 @@ static int sas_discover_new(struct domain_device *dev, int phy_id)
>  	if (res)
>  		return res;
>  	list_for_each_entry(child, &dev->ex_dev.children, siblings) {
> -		if (SAS_ADDR(child->sas_addr) ==
> -		    SAS_ADDR(ex_phy->attached_sas_addr)) {
> +		if (sas_phy_match_dev_addr(child, ex_phy)) {
>  			if (dev_is_expander(child->dev_type))
>  				res = sas_discover_bfs_by_root(child);
>  			break;

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 3/8] scsi: pm8001: use sas_find_attached_phy() instead of open coded

[John Garry]

On 27/09/2022 04:26, Jason Yan wrote:
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -645,22 +645,16 @@ static int pm8001_dev_found_notify(struct domain_device *dev)
>   	pm8001_device->dcompletion = &completion;
>   	if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
>   		int phy_id;
> -		struct ex_phy *phy;
> -		for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
> -		phy_id++) {
> -			phy = &parent_dev->ex_dev.ex_phy[phy_id];
> -			if (SAS_ADDR(phy->attached_sas_addr)
> -				== SAS_ADDR(dev->sas_addr)) {
> -				pm8001_device->attached_phy = phy_id;
> -				break;
> -			}
> -		}
> -		if (phy_id == parent_dev->ex_dev.num_phys) {
> +
> +		phy_id = sas_find_attached_phy(&parent_dev->ex_dev, dev);
> +		if (phy_id == -ENODEV) {
>   			pm8001_dbg(pm8001_ha, FAIL,
>   				   "Error: no attached dev:%016llx at ex:%016llx.\n",
>   				   SAS_ADDR(dev->sas_addr),
>   				   SAS_ADDR(parent_dev->sas_addr));
>   			res = -1;

I think that you can just pass the linux error code (-ENODEV) back here.

And for hisi_sas we change to -EINVAL for this code. I don't think it's 
required, so I think that we can pass -ENODEV back there also. Using 
-EINVAL seems to come from when the code was originally added in 
abda97c2fe874 and from a quick glance libsas does not seem to have 
special processing for -EINVAL.

Thanks,
John

> +		} else {
> +			pm8001_device->attached_phy = phy_id;
>   		}
>   	} else {
>   		if (dev->dev_type == SAS_SATA

Re: [PATCH v4 1/8] scsi: libsas: introduce sas address comparison helpers

[John Garry]

On 27/09/2022 04:25, Jason Yan wrote:
> Sas address comparison is widely used in libsas. However they are all
> opencoded and to avoid the line spill over 80 columns, are mostly split
> into multi-lines. Introduce some helpers to prepare some refactor.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

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

> ---
>   drivers/scsi/libsas/sas_internal.h | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index 8d0ad3abc7b5..3384429b7eb0 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -111,6 +111,23 @@ static inline void sas_smp_host_handler(struct bsg_job *job,
>   }
>   #endif
>   
> +static inline bool sas_phy_match_dev_addr(struct domain_device *dev,
> +					 struct ex_phy *phy)
> +{
> +	return SAS_ADDR(dev->sas_addr) == SAS_ADDR(phy->attached_sas_addr);
> +}
> +
> +static inline bool sas_phy_match_port_addr(struct asd_sas_port *port,
> +					   struct ex_phy *phy)
> +{
> +	return SAS_ADDR(port->sas_addr) == SAS_ADDR(phy->attached_sas_addr);
> +}
> +
> +static inline bool sas_phy_addr_match(struct ex_phy *p1, struct ex_phy *p2)
> +{
> +	return  SAS_ADDR(p1->attached_sas_addr) == SAS_ADDR(p2->attached_sas_addr);
> +}
> +

note: I did think that we could use a macro as a generic matching API, like:

#define sas_phy_match_addr_common(x, ex_phy) \
	({ typeof(x) __dummy; \
	if (__builtin_types_compatible_p(typeof(__dummy), struct asd_sas_port))\
	sas_phy_match_port_addr(x, ex_phy);\
	if (__builtin_types_compatible_p(typeof(__dummy), struct domain_device))\
	sas_phy_match_dev_addr(x, ex_phy);\
	0; \
})

But I couldn't get it to work after 60 minutes of messing ... indeed, 
that latest code doesn't even compile..

Thanks,
John

>   static inline void sas_fail_probe(struct domain_device *dev, const char *func, int err)
>   {
>   	pr_warn("%s: for %s device %016llx returned %d\n",

Re: [PATCH v4 2/8] scsi: libsas: introduce sas_find_attached_phy() helper

[John Garry]

On 27/09/2022 04:25, Jason Yan wrote:
> LLDDs are implementing their own attached phy finding code repeatedly.
> Factor it out to libsas.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>

Apart from nit, below:
Reviewed-by: John Garry <john.garry@huawei.com>

> ---
>   drivers/scsi/libsas/sas_expander.c | 16 ++++++++++++++++
>   include/scsi/libsas.h              |  2 ++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index fa2209080cc2..df5a64ad902f 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -2107,6 +2107,22 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev)
>   	return res;
>   }
>   
> +int sas_find_attached_phy(struct expander_device *ex_dev,
> +			  struct domain_device *dev)

Maybe sas_find_attached_phy_id() is a better name

> +{
> +	struct ex_phy *phy;
> +	int phy_id;
> +
> +	for (phy_id = 0; phy_id < ex_dev->num_phys; phy_id++) {
> +		phy = &ex_dev->ex_phy[phy_id];
> +		if (sas_phy_match_dev_addr(dev, phy))
> +			return phy_id;
> +	}
> +
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(sas_find_attached_phy);
> +
>   void sas_smp_handler(struct bsg_job *job, struct Scsi_Host *shost,
>   		struct sas_rphy *rphy)
>   {
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 2dbead74a2af..75faf2308eae 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -750,6 +750,8 @@ int sas_clear_task_set(struct domain_device *dev, u8 *lun);
>   int sas_lu_reset(struct domain_device *dev, u8 *lun);
>   int sas_query_task(struct sas_task *task, u16 tag);
>   int sas_abort_task(struct sas_task *task, u16 tag);
> +int sas_find_attached_phy(struct expander_device *ex_dev,
> +			  struct domain_device *dev);
>   
>   void sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event,
>   			   gfp_t gfp_flags);

Re: [PATCH v4 3/8] scsi: pm8001: use sas_find_attached_phy() instead of open coded

[Jason Yan]

On 2022/9/27 16:34, John Garry wrote:
> On 27/09/2022 04:26, Jason Yan wrote:
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -645,22 +645,16 @@ static int pm8001_dev_found_notify(struct 
>> domain_device *dev)
>>       pm8001_device->dcompletion = &completion;
>>       if (parent_dev && dev_is_expander(parent_dev->dev_type)) {
>>           int phy_id;
>> -        struct ex_phy *phy;
>> -        for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
>> -        phy_id++) {
>> -            phy = &parent_dev->ex_dev.ex_phy[phy_id];
>> -            if (SAS_ADDR(phy->attached_sas_addr)
>> -                == SAS_ADDR(dev->sas_addr)) {
>> -                pm8001_device->attached_phy = phy_id;
>> -                break;
>> -            }
>> -        }
>> -        if (phy_id == parent_dev->ex_dev.num_phys) {
>> +
>> +        phy_id = sas_find_attached_phy(&parent_dev->ex_dev, dev);
>> +        if (phy_id == -ENODEV) {
>>               pm8001_dbg(pm8001_ha, FAIL,
>>                      "Error: no attached dev:%016llx at ex:%016llx.\n",
>>                      SAS_ADDR(dev->sas_addr),
>>                      SAS_ADDR(parent_dev->sas_addr));
>>               res = -1;
> 
> I think that you can just pass the linux error code (-ENODEV) back here.
> 
> And for hisi_sas we change to -EINVAL for this code. I don't think it's 
> required, so I think that we can pass -ENODEV back there also. Using 
> -EINVAL seems to come from when the code was originally added in 
> abda97c2fe874 and from a quick glance libsas does not seem to have 
> special processing for -EINVAL.
> 

Yes, libsas does not have any special processing for the return value, 
so there is no difference in what value we return here. But I agree with 
you that return -ENODEV is better here because we can keep it consistent 
with the return value of sas_find_attached_phy().

Will update.

Thanks,
Jason

> Thanks,
> John
> 
>> +        } else {
>> +            pm8001_device->attached_phy = phy_id;
>>           }
>>       } else {
>>           if (dev->dev_type == SAS_SATA
> 
> .