Messages in this thread | | | Date | Fri, 23 Sep 2022 11:00:52 +0100 | Subject | Re: [PATCH 6/7] scsi: pm8001: use dev_and_phy_addr_same() instead of open coded | From | John Garry <> |
| |
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
| |