[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6c299e8f-80be-0276-c8b1-9df1946434da@huawei.com>
Date: Fri, 23 Sep 2022 11:00:52 +0100
From: John Garry <john.garry@...wei.com>
To: Jason Yan <yanaijie@...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 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
Powered by blists - more mailing lists