2022-09-27 03:20:24

by Jason Yan

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

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


2022-09-27 03:29:09

by Jason Yan

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

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

Signed-off-by: Jason Yan <[email protected]>
Reviewed-by: Jack Wang <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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

2022-09-27 03:34:38

by Jason Yan

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

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 <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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

2022-09-27 03:35:32

by Jason Yan

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

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

Signed-off-by: Jason Yan <[email protected]>
Reviewed-by: Jack Wang <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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

2022-09-27 03:35:43

by Jason Yan

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

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 <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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

2022-09-27 03:35:53

by Jason Yan

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

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

Signed-off-by: Jason Yan <[email protected]>
Reviewed-by: Jack Wang <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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

2022-09-27 03:51:34

by Jason Yan

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

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 <[email protected]>
---
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

2022-09-27 04:02:26

by Jason Yan

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

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

Signed-off-by: Jason Yan <[email protected]>
Reviewed-by: Jack Wang <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
---
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

2022-09-27 04:51:15

by Damien Le Moal

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

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 <[email protected]>

Reviewed-by: Damien Le Moal <[email protected]>

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

2022-09-27 08:38:31

by John Garry

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

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

2022-09-27 09:49:34

by John Garry

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

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 <[email protected]>
> Reviewed-by: Jack Wang <[email protected]>
> Reviewed-by: Damien Le Moal <[email protected]>

Apart from nit, below:
Reviewed-by: John Garry <[email protected]>

> ---
> 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);

2022-09-27 12:44:42

by Jason Yan

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


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