On Wed, 2 Apr 2025, Hans Zhang wrote:
> On 2025/4/2 20:38, Ilpo Järvinen wrote:
> > On Wed, 2 Apr 2025, Hans Zhang wrote:
> >
> > > Refactor the PCI capability and extended capability search functions
> > > by consolidating common code patterns into reusable macros
> > > (PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
> > > changes include:
> > >
> > > 1. Introducing a unified config space read helper (__pci_bus_read_config).
> > > 2. Removing duplicate search logic from __pci_find_next_cap_ttl and
> > > pci_find_next_ext_capability.
> > > 3. Implementing consistent capability discovery using the new macros.
> > > 4. Simplifying HyperTransport capability lookup by leveraging the
> > > refactored code.
> > >
> > > The refactoring maintains existing functionality while reducing code
> > > duplication and improving maintainability. By centralizing the search
> > > logic, we achieve better code consistency and make future updates easier.
> > >
> > > This change has been verified to maintain backward compatibility with
> > > existing capability discovery patterns through thorough testing of PCI
> > > device enumeration and capability probing.
> > >
> > > Signed-off-by: Hans Zhang <18255117159@xxxxxxx>
> > > ---
> > > drivers/pci/pci.c | 79 +++++++++++++----------------------------------
> > > 1 file changed, 22 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 869d204a70a3..521096c73686 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -423,36 +423,33 @@ static int pci_dev_str_match(struct pci_dev *dev,
> > > const char *p,
> > > return 1;
> > > }
> > > -static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int
> > > devfn,
> > > - u8 pos, int cap, int *ttl)
> > > +static int __pci_bus_read_config(void *priv, unsigned int devfn, int
> > > where,
> > > + u32 size, u32 *val)
> >
> > This probably should be where the other accessors are so in access.c. I'd
> > put its prototype into drivers/pci/pci.h only for now.
> >
>
> Hi Ilpo,
>
> Thank you very much for your guidance. Will change.
>
>
> > > {
> > > - u8 id;
> > > - u16 ent;
> > > + struct pci_bus *bus = priv;
> > > + int ret;
> > > - pci_bus_read_config_byte(bus, devfn, pos, &pos);
> > > + if (size == 1)
> > > + ret = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> > > + else if (size == 2)
> > > + ret = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> > > + else
> > > + ret = pci_bus_read_config_dword(bus, devfn, where, val);
> > > - while ((*ttl)--) {
> > > - if (pos < 0x40)
> > > - break;
> > > - pos &= ~3;
> > > - pci_bus_read_config_word(bus, devfn, pos, &ent);
> > > + return ret;
> > > +}
> > > - id = ent & 0xff;
> > > - if (id == 0xff)
> > > - break;
> > > - if (id == cap)
> > > - return pos;
> > > - pos = (ent >> 8);
> > > - }
> > > - return 0;
> > > +static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int
> > > devfn,
> > > + u8 pos, int cap)
> > > +{
> > > + return PCI_FIND_NEXT_CAP_TTL(__pci_bus_read_config, pos, cap, bus,
> > > + devfn);
> > > }
> > > static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> > > u8 pos, int cap)
> > > {
> > > - int ttl = PCI_FIND_CAP_TTL;
> > > -
> > > - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> > > + return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
> > > }
> > > u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> > > @@ -553,42 +550,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
> > > */
> > > u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int
> > > cap)
> > > {
> > > - u32 header;
> > > - int ttl;
> > > - u16 pos = PCI_CFG_SPACE_SIZE;
> > > -
> > > - /* minimum 8 bytes per capability */
> > > - ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> > > -
> > > if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> > > return 0;
> > > - if (start)
> > > - pos = start;
> > > -
> > > - if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> > > - return 0;
> > > -
> > > - /*
> > > - * If we have no capabilities, this is indicated by cap ID,
> > > - * cap version and next pointer all being 0.
> > > - */
> > > - if (header == 0)
> > > - return 0;
> > > -
> > > - while (ttl-- > 0) {
> > > - if (PCI_EXT_CAP_ID(header) == cap && pos != start)
> > > - return pos;
> > > -
> > > - pos = PCI_EXT_CAP_NEXT(header);
> > > - if (pos < PCI_CFG_SPACE_SIZE)
> > > - break;
> > > -
> > > - if (pci_read_config_dword(dev, pos, &header) !=
> > > PCIBIOS_SUCCESSFUL)
> > > - break;
> > > - }
> > > -
> > > - return 0;
> > > + return PCI_FIND_NEXT_EXT_CAPABILITY(__pci_bus_read_config, start, cap,
> > > + dev->bus, dev->devfn);
> >
> > I don't like how 1 & 2 patches are split into two. IMO, they mostly belong
> > together. However, (IMO) you can introduce the new all-size config space
> > accessor in a separate patch before the combined patch.
> >
>
> Ok. I'll change it to the following. The rest I'll combine into a patch.
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63b..bb2e26c2eb81 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
> EXPORT_SYMBOL(pci_bus_write_config_word);
> EXPORT_SYMBOL(pci_bus_write_config_dword);
>
> +
Extra newline
> +int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
> + u32 *val)
> +{
> + struct pci_bus *bus = priv;
> + int ret;
> +
> + if (size == 1)
> + ret = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> + else if (size == 2)
> + ret = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> + else
> + ret = pci_bus_read_config_dword(bus, devfn, where, val);
> +
> + return ret;
> +}
> +
> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> int where, int size, u32 *val)
> {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2e9cf26a9ee9..6a7c88b9cd35 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -88,6 +88,8 @@ extern bool pci_early_dump;
> bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
> bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> +int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
> + u32 *val);
>
> /* Functions internal to the PCI core code */
>
>
> > > }
> > > EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
> > > @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
> > > static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int
> > > ht_cap)
> > > {
> > > - int rc, ttl = PCI_FIND_CAP_TTL;
> > > u8 cap, mask;
> > > if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> > > @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
> > > u8 pos, int ht_cap)
> > > mask = HT_5BIT_CAP_MASK;
> > > pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> > > - PCI_CAP_ID_HT, &ttl);
> > > + PCI_CAP_ID_HT);
> > > while (pos) {
> > > rc = pci_read_config_byte(dev, pos + 3, &cap);
> > > if (rc != PCIBIOS_SUCCESSFUL)
> > > @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
> > > u8 pos, int ht_cap)
> > > pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> > > pos + PCI_CAP_LIST_NEXT,
> > > - PCI_CAP_ID_HT, &ttl);
> > > + PCI_CAP_ID_HT);
> >
> > This function kind of had the idea to share the ttl but I suppose that was
> > just a final safeguard to make sure the loop will always terminate in case
> > the config space is corrupted so the unsharing is not a big issue.
> >
>
> __pci_find_next_cap_ttl
> // This macro definition already has ttl loop restrictions inside it.
> PCI_FIND_NEXT_CAP_TTL
>
> Do I understand that you agree to remove ttl initialization and parameter
> passing?
Yes, I agree with it but doing anything like this (although I'd mention
the reasoning in the changelog myself).
--
i.
