[PATCH 0/7] scsi: libsas: sas address comparation refactor

[PATCH 0/7] scsi: libsas: sas address comparation refactor

[Jason Yan]

Sas address conversion and comparation 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.

Jason Yan (7):
  scsi: libsas: introduce sas address conversion and comparation helpers
  scsi: libsas: use dev_and_phy_addr_same() instead of open coded
  scsi: libsas: use ex_phy_addr_same() instead of open coded
  scsi: libsas: use port_and_phy_addr_same() instead of open coded
  scsi: hisi_sas: use dev_and_phy_addr_same() instead of open coded
  scsi: pm8001: use dev_and_phy_addr_same() instead of open coded
  scsi: mvsas: use dev_and_phy_addr_same() instead of open coded

 drivers/scsi/hisi_sas/hisi_sas_main.c |  3 +--
 drivers/scsi/libsas/sas_expander.c    | 24 +++++++-------------
 drivers/scsi/mvsas/mv_sas.c           |  3 +--
 drivers/scsi/pm8001/pm8001_sas.c      |  3 +--
 include/scsi/libsas.h                 | 32 +++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 22 deletions(-)

-- 
2.31.1

[PATCH 1/7] scsi: libsas: introduce sas address conversion and comparation helpers

[Jason Yan]

Sas address conversion and comparation 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. Introduce some helpers to
prepare some refactor.

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 include/scsi/libsas.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2dbead74a2af..382aedf31fa4 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -648,6 +648,38 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
 	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
 }
 
+static inline unsigned long long ex_phy_addr(struct ex_phy *phy)
+{
+	return SAS_ADDR(phy->attached_sas_addr);
+}
+
+static inline unsigned long long dev_addr(struct domain_device *dev)
+{
+	return SAS_ADDR(dev->sas_addr);
+}
+
+static inline unsigned long long port_addr(struct asd_sas_port *port)
+{
+	return SAS_ADDR(port->sas_addr);
+}
+
+static inline bool dev_and_phy_addr_same(struct domain_device *dev,
+					 struct ex_phy *phy)
+{
+	return dev_addr(dev) == ex_phy_addr(phy);
+}
+
+static inline bool port_and_phy_addr_same(struct asd_sas_port *port,
+					  struct ex_phy *phy)
+{
+	return port_addr(port) == ex_phy_addr(phy);
+}
+
+static inline bool ex_phy_addr_same(struct ex_phy *phy1, struct ex_phy *phy2)
+{
+	return  ex_phy_addr(phy1) ==  ex_phy_addr(phy2);
+}
+
 struct sas_domain_function_template {
 	/* The class calls these to notify the LLDD of an event. */
 	void (*lldd_port_formed)(struct asd_sas_phy *);
-- 
2.31.1

[PATCH 2/7] scsi: libsas: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

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

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 fa2209080cc2..dbf9ffa8367c 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 (dev_and_phy_addr_same(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 && dev_and_phy_addr_same(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 (dev_and_phy_addr_same(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))) {
+		    dev_and_phy_addr_same(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 (dev_and_phy_addr_same(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 (dev_and_phy_addr_same(child, ex_phy)) {
 			if (dev_is_expander(child->dev_type))
 				res = sas_discover_bfs_by_root(child);
 			break;
-- 
2.31.1

[PATCH 3/7] scsi: libsas: use ex_phy_addr_same() instead of open coded

[Jason Yan]

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

Signed-off-by: Jason Yan <yanaijie@huawei.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 dbf9ffa8367c..18b008f039be 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 (ex_phy_addr_same(phy, changed_phy)) {
 				last = false;
 				break;
 			}
-- 
2.31.1

[PATCH 4/7] scsi: libsas: use port_and_phy_addr_same() instead of open coded

[Jason Yan]

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

Signed-off-by: Jason Yan <yanaijie@huawei.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 18b008f039be..667b276caeee 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 && port_and_phy_addr_same(dev->port, ex_phy)) {
 		sas_add_parent_port(dev, phy_id);
 		return 0;
 	}
-- 
2.31.1

[PATCH 5/7] scsi: hisi_sas: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

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

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 33af5b8dede2..4a11b717d03b 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -797,8 +797,7 @@ static int hisi_sas_dev_found(struct domain_device *device)
 
 		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))
+			if (dev_and_phy_addr_same(device, phy))
 				break;
 		}
 
-- 
2.31.1

[PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

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

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/pm8001/pm8001_sas.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 8e3f2f9ddaac..bb1b1722f3ee 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -649,8 +649,7 @@ static int pm8001_dev_found_notify(struct domain_device *dev)
 		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)) {
+			if (dev_and_phy_addr_same(dev, phy)) {
 				pm8001_device->attached_phy = phy_id;
 				break;
 			}
-- 
2.31.1

[PATCH 7/7] scsi: mvsas: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

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

Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 drivers/scsi/mvsas/mv_sas.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index a6867dae0e7c..766e02c3f459 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -1194,8 +1194,7 @@ static int mvs_dev_found_notify(struct domain_device *dev, int lock)
 		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)) {
+			if (dev_and_phy_addr_same(dev, phy)) {
 				mvi_device->attached_phy = phy_id;
 				break;
 			}
-- 
2.31.1

Re: [PATCH 1/7] scsi: libsas: introduce sas address conversion and comparation helpers

[John Garry]

On 17/09/2022 11:43, Jason Yan wrote:
> Sas address conversion and comparation 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. Introduce some helpers to
> prepare some refactor.
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   include/scsi/libsas.h | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 2dbead74a2af..382aedf31fa4 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -648,6 +648,38 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>   	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>   }
>   
> +static inline unsigned long long ex_phy_addr(struct ex_phy *phy)

This is a public header, so I would hope that any function would have 
"sas_" prefix

> +{
> +	return SAS_ADDR(phy->attached_sas_addr);
> +}
> +
> +static inline unsigned long long dev_addr(struct domain_device *dev)
> +{
> +	return SAS_ADDR(dev->sas_addr);
> +}
> +
> +static inline unsigned long long port_addr(struct asd_sas_port *port)
> +{
> +	return SAS_ADDR(port->sas_addr);

As below, I don't really see how these simple functions help much

> +}
> +
> +static inline bool dev_and_phy_addr_same(struct domain_device *dev,
> +					 struct ex_phy *phy)
> +{
> +	return dev_addr(dev) == ex_phy_addr(phy);
> +}
> +
> +static inline bool port_and_phy_addr_same(struct asd_sas_port *port,
> +					  struct ex_phy *phy)

I'd say sas_phy_match_port_addr() could be a better name.

> +{
> +	return port_addr(port) == ex_phy_addr(phy);

I think the following is just as good:

	return SAS_ADDR(port->sas_addr) == SAS_ADDR(phy->attached_sas_addr)

port_addr() is only used once AFAICS, so the code would not be less concise

> +}
> +
> +static inline bool ex_phy_addr_same(struct ex_phy *phy1, struct ex_phy *phy2)
> +{
> +	return  ex_phy_addr(phy1) ==  ex_phy_addr(phy2);

nit: 2x double whitespace

> +}
> +
>   struct sas_domain_function_template {
>   	/* The class calls these to notify the LLDD of an event. */
>   	void (*lldd_port_formed)(struct asd_sas_phy *);

Thanks,
John

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[John Garry]

On 17/09/2022 11:43, Jason Yan wrote:
> The sas address comparation of domain device and expander phy is open
> coded. Now we can replace it with dev_and_phy_addr_same().
> 
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>   drivers/scsi/pm8001/pm8001_sas.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 8e3f2f9ddaac..bb1b1722f3ee 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -649,8 +649,7 @@ static int pm8001_dev_found_notify(struct domain_device *dev)
>   		for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;

This code seems the same between many libsas LLDDs - could we factor it 
out into libsas? If so, then maybe those new helpers could be put in 
sas_internal.h

Thanks,
John

>   		phy_id++) {
>   			phy = &parent_dev->ex_dev.ex_phy[phy_id];
> -			if (SAS_ADDR(phy->attached_sas_addr)
> -				== SAS_ADDR(dev->sas_addr)) {
> +			if (dev_and_phy_addr_same(dev, phy)) {
>   				pm8001_device->attached_phy = phy_id;
>   				break;
>   			}

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

On 2022/9/22 22:24, John Garry wrote:
> On 17/09/2022 11:43, Jason Yan wrote:
>> The sas address comparation of domain device and expander phy is open
>> coded. Now we can replace it with dev_and_phy_addr_same().
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>>   drivers/scsi/pm8001/pm8001_sas.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
>> b/drivers/scsi/pm8001/pm8001_sas.c
>> index 8e3f2f9ddaac..bb1b1722f3ee 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -649,8 +649,7 @@ static int pm8001_dev_found_notify(struct 
>> domain_device *dev)
>>           for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
> 
> This code seems the same between many libsas LLDDs - could we factor it 
> out into libsas?

Sure we can. I will try to factor it out in the next revision.

Thanks,
Jason

  If so, then maybe those new helpers could be put in
> sas_internal.h
> 
> Thanks,
> John
> 
>>           phy_id++) {
>>               phy = &parent_dev->ex_dev.ex_phy[phy_id];
>> -            if (SAS_ADDR(phy->attached_sas_addr)
>> -                == SAS_ADDR(dev->sas_addr)) {
>> +            if (dev_and_phy_addr_same(dev, phy)) {
>>                   pm8001_device->attached_phy = phy_id;
>>                   break;
>>               }
> 
> .

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

On 2022/9/22 22:24, John Garry wrote:
> On 17/09/2022 11:43, Jason Yan wrote:
>> The sas address comparation of domain device and expander phy is open
>> coded. Now we can replace it with dev_and_phy_addr_same().
>>
>> Signed-off-by: Jason Yan <yanaijie@huawei.com>
>> ---
>>   drivers/scsi/pm8001/pm8001_sas.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
>> b/drivers/scsi/pm8001/pm8001_sas.c
>> index 8e3f2f9ddaac..bb1b1722f3ee 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -649,8 +649,7 @@ static int pm8001_dev_found_notify(struct 
>> domain_device *dev)
>>           for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
> 
> This code seems the same between many libsas LLDDs - could we factor it 
> out into libsas? If so, then maybe those new helpers could be put in 
> sas_internal.h

For the part of putting helpers in sas_internal.h, this needs to make 
the helpers exported. I think it's not worth to do this because they are 
very small. I'd still like to make them inline functions in libsas.h 
such as:


diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 2dbead74a2af..e9e76c898287 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -648,6 +648,22 @@ static inline bool sas_is_internal_abort(struct 
sas_task *task)
         return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
  }

+static inline int sas_find_attathed_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_ADDR(phy->attached_sas_addr)
+                       == SAS_ADDR(dev->sas_addr))
+                       return phy_id;
+       }
+
+       return ex_dev->num_phys;
+}
+
  struct sas_domain_function_template {
         /* The class calls these to notify the LLDD of an event. */
         void (*lldd_port_formed)(struct asd_sas_phy *);



And the LLDDs change like:


diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
b/drivers/scsi/pm8001/pm8001_sas.c
index 8e3f2f9ddaac..4e7350609b3d 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -645,16 +645,8 @@ 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;
-                       }
-               }
+
+               phy_id = sas_find_attathed_phy(&parent_dev->ex_dev, dev);
                 if (phy_id == parent_dev->ex_dev.num_phys) {
                         pm8001_dbg(pm8001_ha, FAIL,
                                    "Error: no attached dev:%016llx at 
ex:%016llx.\n",
@@ -662,6 +654,7 @@ static int pm8001_dev_found_notify(struct 
domain_device *dev)
                                    SAS_ADDR(parent_dev->sas_addr));
                         res = -1;
                 }
+               pm8001_device->attached_phy = phy_id;
         } else {
                 if (dev->dev_type == SAS_SATA_DEV) {
                         pm8001_device->attached_phy =


So I wonder if you have any reasons to insist exporting the helper?

> 
> Thanks,
> John
> 
>>           phy_id++) {
>>               phy = &parent_dev->ex_dev.ex_phy[phy_id];
>> -            if (SAS_ADDR(phy->attached_sas_addr)
>> -                == SAS_ADDR(dev->sas_addr)) {
>> +            if (dev_and_phy_addr_same(dev, phy)) {
>>                   pm8001_device->attached_phy = phy_id;
>>                   break;
>>               }
> 
> .

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[John Garry]

On 23/09/2022 10:44, Jason Yan wrote:
>>>           for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
>>
>> This code seems the same between many libsas LLDDs - could we factor 
>> it out into libsas? If so, then maybe those new helpers could be put 
>> in sas_internal.h
> 
> For the part of putting helpers in sas_internal.h, this needs to make 
> the helpers exported.

Please explain why.

I would assume that if those helpers were only used in libsas code (and 
not LLDDs) then they could be put in sas_internal.h and no need for export

> I think it's not worth to do this because they are 
> very small. I'd still like to make them inline functions in libsas.h 
> such as:
> 
> 
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 2dbead74a2af..e9e76c898287 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -648,6 +648,22 @@ static inline bool sas_is_internal_abort(struct 
> sas_task *task)
>          return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>   }
> 
> +static inline int sas_find_attathed_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_ADDR(phy->attached_sas_addr)
> +                       == SAS_ADDR(dev->sas_addr))
> +                       return phy_id;
> +       }
> +
> +       return ex_dev->num_phys;

I will note that this code does not use your new helpers

> +}
> +
>   struct sas_domain_function_template {
>          /* The class calls these to notify the LLDD of an event. */
>          void (*lldd_port_formed)(struct asd_sas_phy *);
> 
> 
> 
> And the LLDDs change like:
> 
> 
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
> b/drivers/scsi/pm8001/pm8001_sas.c
> index 8e3f2f9ddaac..4e7350609b3d 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -645,16 +645,8 @@ 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;
> -                       }
> -               }
> +
> +               phy_id = sas_find_attathed_phy(&parent_dev->ex_dev, dev);
>                  if (phy_id == parent_dev->ex_dev.num_phys) {
>                          pm8001_dbg(pm8001_ha, FAIL,
>                                     "Error: no attached dev:%016llx at 
> ex:%016llx.\n",
> @@ -662,6 +654,7 @@ static int pm8001_dev_found_notify(struct 
> domain_device *dev)
>                                     SAS_ADDR(parent_dev->sas_addr));
>                          res = -1;
>                  }
> +               pm8001_device->attached_phy = phy_id;
>          } else {
>                  if (dev->dev_type == SAS_SATA_DEV) {
>                          pm8001_device->attached_phy =
> 
> 
> So I wonder if you have any reasons to insist exporting the helper
Thanks,
John

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

On 2022/9/23 18:00, John Garry wrote:
> On 23/09/2022 10:44, Jason Yan wrote:
>>>>           for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
>>>
>>> This code seems the same between many libsas LLDDs - could we factor 
>>> it out into libsas? If so, then maybe those new helpers could be put 
>>> in sas_internal.h
>>
>> For the part of putting helpers in sas_internal.h, this needs to make 
>> the helpers exported.
> 
> Please explain why.
> 
> I would assume that if those helpers were only used in libsas code (and 
> not LLDDs) then they could be put in sas_internal.h and no need for export
> 
>> I think it's not worth to do this because they are very small. I'd 
>> still like to make them inline functions in libsas.h such as:
>>
>>
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index 2dbead74a2af..e9e76c898287 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -648,6 +648,22 @@ static inline bool sas_is_internal_abort(struct 
>> sas_task *task)
>>          return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>>   }
>>
>> +static inline int sas_find_attathed_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_ADDR(phy->attached_sas_addr)
>> +                       == SAS_ADDR(dev->sas_addr))
>> +                       return phy_id;
>> +       }
>> +
>> +       return ex_dev->num_phys;
> 
> I will note that this code does not use your new helpers

NO, this code above will use my new helpers.(Unless we skip this part)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 635039557bdb..9283462704f0 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -673,8 +673,7 @@ static inline int sas_find_attathed_phy(struct 
expander_device *ex_dev,

         for (phy_id = 0; phy_id < ex_dev->num_phys; phy_id++) {
                 phy = &ex_dev->ex_phy[phy_id];
-               if (SAS_ADDR(phy->attached_sas_addr)
-                       == SAS_ADDR(dev->sas_addr))
+               if (sas_phy_match_dev_addr(dev, phy))
                         return phy_id;




> 
>> +}
>> +
>>   struct sas_domain_function_template {
>>          /* The class calls these to notify the LLDD of an event. */
>>          void (*lldd_port_formed)(struct asd_sas_phy *);
>>
>>
>>
>> And the LLDDs change like:
>>
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
>> b/drivers/scsi/pm8001/pm8001_sas.c
>> index 8e3f2f9ddaac..4e7350609b3d 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -645,16 +645,8 @@ 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;
>> -                       }
>> -               }
>> +
>> +               phy_id = sas_find_attathed_phy(&parent_dev->ex_dev, dev);
>>                  if (phy_id == parent_dev->ex_dev.num_phys) {
>>                          pm8001_dbg(pm8001_ha, FAIL,
>>                                     "Error: no attached dev:%016llx at 
>> ex:%016llx.\n",
>> @@ -662,6 +654,7 @@ static int pm8001_dev_found_notify(struct 
>> domain_device *dev)
>>                                     SAS_ADDR(parent_dev->sas_addr));
>>                          res = -1;
>>                  }
>> +               pm8001_device->attached_phy = phy_id;
>>          } else {
>>                  if (dev->dev_type == SAS_SATA_DEV) {
>>                          pm8001_device->attached_phy =
>>
>>
>> So I wonder if you have any reasons to insist exporting the helper
> Thanks,
> John
> .

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

On 2022/9/23 18:00, John Garry wrote:
> On 23/09/2022 10:44, Jason Yan wrote:
>>>>           for (phy_id = 0; phy_id < parent_dev->ex_dev.num_phys;
>>>
>>> This code seems the same between many libsas LLDDs - could we factor 
>>> it out into libsas? If so, then maybe those new helpers could be put 
>>> in sas_internal.h
>>
>> For the part of putting helpers in sas_internal.h, this needs to make 
>> the helpers exported.
> 
> Please explain why.
> 
> I would assume that if those helpers were only used in libsas code (and 
> not LLDDs) then they could be put in sas_internal.h and no need for export
> 


Sorry, I did not make it clear. I mean we need to export 
sas_find_attathed_phy() below. Not the sas address comparation helpers.



>> I think it's not worth to do this because they are very small. I'd 
>> still like to make them inline functions in libsas.h such as:
>>
>>
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index 2dbead74a2af..e9e76c898287 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -648,6 +648,22 @@ static inline bool sas_is_internal_abort(struct 
>> sas_task *task)
>>          return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>>   }
>>
>> +static inline int sas_find_attathed_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_ADDR(phy->attached_sas_addr)
>> +                       == SAS_ADDR(dev->sas_addr))
>> +                       return phy_id;
>> +       }
>> +
>> +       return ex_dev->num_phys;
> 
> I will note that this code does not use your new helpers
> 
>> +}
>> +
>>   struct sas_domain_function_template {
>>          /* The class calls these to notify the LLDD of an event. */
>>          void (*lldd_port_formed)(struct asd_sas_phy *);
>>
>>
>>
>> And the LLDDs change like:
>>
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c 
>> b/drivers/scsi/pm8001/pm8001_sas.c
>> index 8e3f2f9ddaac..4e7350609b3d 100644
>> --- a/drivers/scsi/pm8001/pm8001_sas.c
>> +++ b/drivers/scsi/pm8001/pm8001_sas.c
>> @@ -645,16 +645,8 @@ 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;
>> -                       }
>> -               }
>> +
>> +               phy_id = sas_find_attathed_phy(&parent_dev->ex_dev, dev);
>>                  if (phy_id == parent_dev->ex_dev.num_phys) {
>>                          pm8001_dbg(pm8001_ha, FAIL,
>>                                     "Error: no attached dev:%016llx at 
>> ex:%016llx.\n",
>> @@ -662,6 +654,7 @@ static int pm8001_dev_found_notify(struct 
>> domain_device *dev)
>>                                     SAS_ADDR(parent_dev->sas_addr));
>>                          res = -1;
>>                  }
>> +               pm8001_device->attached_phy = phy_id;
>>          } else {
>>                  if (dev->dev_type == SAS_SATA_DEV) {
>>                          pm8001_device->attached_phy =
>>
>>
>> So I wonder if you have any reasons to insist exporting the helper
> Thanks,
> John
> .

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[John Garry]

On 23/09/2022 11:13, Jason Yan wrote:
>>
>> Please explain why.
>>
>> I would assume that if those helpers were only used in libsas code 
>> (and not LLDDs) then they could be put in sas_internal.h and no need 
>> for export
>>
> 
> 
> Sorry, I did not make it clear. I mean we need to export 
> sas_find_attathed_phy() below. Not the sas address comparation helpers.

That seems fine to me.

About sas_find_attathed_phy() implementation,

 > +static inline int sas_find_attathed_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_ADDR(phy->attached_sas_addr)
 > +                       == SAS_ADDR(dev->sas_addr))
 > +                       return phy_id;
 > +       }
 > +
 > +       return ex_dev->num_phys;

Returning ex_dev->num_phys would seem ok, but then the LLDD needs to 
check that return against ex_dev->num_phys. It seems ok, but I'm still 
not 100% comfortable with that. Maybe returning -ENODEV may be better.

Or return boolean and pass phy_id as pointer to be filled in when 
returning true.

 > +}

Thanks,
John

Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded

[Jason Yan]

On 2022/9/23 18:30, John Garry wrote:
> On 23/09/2022 11:13, Jason Yan wrote:
>>>
>>> Please explain why.
>>>
>>> I would assume that if those helpers were only used in libsas code 
>>> (and not LLDDs) then they could be put in sas_internal.h and no need 
>>> for export
>>>
>>
>>
>> Sorry, I did not make it clear. I mean we need to export 
>> sas_find_attathed_phy() below. Not the sas address comparation helpers.
> 
> That seems fine to me.
> 
> About sas_find_attathed_phy() implementation,
> 
>  > +static inline int sas_find_attathed_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_ADDR(phy->attached_sas_addr)
>  > +                       == SAS_ADDR(dev->sas_addr))
>  > +                       return phy_id;
>  > +       }
>  > +
>  > +       return ex_dev->num_phys;
> 
> Returning ex_dev->num_phys would seem ok, but then the LLDD needs to 
> check that return against ex_dev->num_phys. It seems ok, but I'm still 
> not 100% comfortable with that. Maybe returning -ENODEV may be better.
> 
> Or return boolean and pass phy_id as pointer to be filled in when 
> returning true.
> 

I've been thinking about this for a while too. Thank you for the advise.

Thanks,
Jason

>  > +}
> 
> Thanks,
> John
> 
> .