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.
v4->v5:
Rename sas_find_attached_phy() to sas_find_attached_phy_id().
Return error code from sas_find_attached_phy_id() directly.
Add review tags from John and Damien.
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_id() helper
scsi: pm8001: use sas_find_attached_phy_id() instead of open coded
scsi: mvsas: use sas_find_attached_phy_id() instead of open coded
scsi: hisi_sas: use sas_find_attathed_phy_id() 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 | 14 ++--------
drivers/scsi/libsas/sas_expander.c | 40 ++++++++++++++++-----------
drivers/scsi/libsas/sas_internal.h | 17 ++++++++++++
drivers/scsi/mvsas/mv_sas.c | 17 ++++--------
drivers/scsi/pm8001/pm8001_sas.c | 18 ++++--------
include/scsi/libsas.h | 2 ++
6 files changed, 57 insertions(+), 51 deletions(-)
--
2.31.1
The attached phy finding is open coded. Now we can replace it with
sas_find_attached_phy_id(). To keep consistent, the return value of
hisi_sas_dev_found() is also changed to -ENODEV after calling
sas_find_attathed_phy_id() failed.
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 | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 33af5b8dede2..4bb0416a2f65 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -792,22 +792,14 @@ 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_id(&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),
SAS_ADDR(parent_dev->sas_addr));
- rc = -EINVAL;
+ rc = phy_no;
goto err_out;
}
}
--
2.31.1
The attached phy id finding is open coded. Now we can replace it with
sas_find_attached_phy_id(). To keep consistent, the return value of
pm8001_dev_found_notify() is also changed to -ENODEV after calling
sas_find_attathed_phy_id() failed.
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 | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 8e3f2f9ddaac..042c0843de1a 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_id(&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;
+ res = phy_id;
+ } else {
+ pm8001_device->attached_phy = phy_id;
}
} else {
if (dev->dev_type == SAS_SATA_DEV) {
--
2.31.1
The attached phy finding is open coded. Now we can replace it with
sas_find_attached_phy_id(). To keep consistent, the return value of
mvs_dev_found_notify() is also changed to -ENODEV after calling
sas_find_attathed_phy_id() failed.
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 | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index a6867dae0e7c..4baf4973f241 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_id(&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;
+ res = phy_id;
+ } else {
+ mvi_device->attached_phy = phy_id;
}
}
--
2.31.1
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 38ed42b14fa0..b358a1cd9a68 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
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 2caf366b9f74..9393ab83358c 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
LLDDs are implementing their own attached phy id 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]>
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..2caf366b9f74 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_id(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_id);
+
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..ec08008b919c 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_id(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
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 9393ab83358c..38ed42b14fa0 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
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 <[email protected]>
Reviewed-by: Damien Le Moal <[email protected]>
Reviewed-by: John Garry <[email protected]>
---
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
On 9/27/22 21:39, Jason Yan wrote:
> The attached phy id finding is open coded. Now we can replace it with
> sas_find_attached_phy_id(). To keep consistent, the return value of
> pm8001_dev_found_notify() is also changed to -ENODEV after calling
> sas_find_attathed_phy_id() failed.
>
> 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 | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 8e3f2f9ddaac..042c0843de1a 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_id(&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;
> + res = phy_id;
Nit:
res = -ENODEV would be a lot clearer.
Or do:
if (phy_id < 0) {
...
ret = phy_id;
} ...
No ?
> + } else {
> + pm8001_device->attached_phy = phy_id;
> }
> } else {
> if (dev->dev_type == SAS_SATA_DEV) {
--
Damien Le Moal
Western Digital Research
On 2022/9/28 6:57, Damien Le Moal wrote:
> On 9/27/22 21:39, Jason Yan wrote:
>> The attached phy id finding is open coded. Now we can replace it with
>> sas_find_attached_phy_id(). To keep consistent, the return value of
>> pm8001_dev_found_notify() is also changed to -ENODEV after calling
>> sas_find_attathed_phy_id() failed.
>>
>> 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 | 18 ++++++------------
>> 1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>> index 8e3f2f9ddaac..042c0843de1a 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_id(&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;
>> + res = phy_id;
>
> Nit:
>
> res = -ENODEV would be a lot clearer.
> Or do:
>
> if (phy_id < 0) {
> ...
> ret = phy_id;
> } ...
>
This boils down to personal preferences. I'd like to change to the
latter one if no objections.
Thanks,
Jason
> No ?
>
>> + } else {
>> + pm8001_device->attached_phy = phy_id;
>> }
>> } else {
>> if (dev->dev_type == SAS_SATA_DEV) {
>
On 9/28/22 11:17, Jason Yan wrote:
>
> On 2022/9/28 6:57, Damien Le Moal wrote:
>> On 9/27/22 21:39, Jason Yan wrote:
>>> The attached phy id finding is open coded. Now we can replace it with
>>> sas_find_attached_phy_id(). To keep consistent, the return value of
>>> pm8001_dev_found_notify() is also changed to -ENODEV after calling
>>> sas_find_attathed_phy_id() failed.
>>>
>>> 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 | 18 ++++++------------
>>> 1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
>>> index 8e3f2f9ddaac..042c0843de1a 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_id(&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;
>>> + res = phy_id;
>>
>> Nit:
>>
>> res = -ENODEV would be a lot clearer.
>> Or do:
>>
>> if (phy_id < 0) {
>> ...
>> ret = phy_id;
>> } ...
>>
>
> This boils down to personal preferences. I'd like to change to the
> latter one if no objections.
Either work for me. The point is to preferably have something consistent
with the return value from sas_find_attached_phy_id() and not playing
games with it. So yes, the second one is fine.
>
> Thanks,
> Jason
>
>> No ?
>>
>>> + } else {
>>> + pm8001_device->attached_phy = phy_id;
>>> }
>>> } else {
>>> if (dev->dev_type == SAS_SATA_DEV) {
>>
--
Damien Le Moal
Western Digital Research