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.1On 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;
> }
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;
>> }
>
> .
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
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
> .
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
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
>
> .
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
> .
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;
>> }
>
> .
© 2016 - 2026 Red Hat, Inc.