[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <93c7a2e5-60e8-bafc-b35d-60b7d5972b95@huawei.com>
Date: Fri, 23 Sep 2022 18:10:41 +0800
From: Jason Yan <yanaijie@...wei.com>
To: John Garry <john.garry@...wei.com>, <martin.petersen@...cle.com>,
<jejb@...ux.ibm.com>
CC: <linux-scsi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<hare@...e.com>, <hch@....de>, <bvanassche@....org>,
<jinpu.wang@...ud.ionos.com>
Subject: Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of
open coded
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
> .
Powered by blists - more mailing lists