Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication

[Hans Zhang]

On 2025/4/3 17:15, Ilpo Järvinen wrote:
> > > 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

Hi Ilpo,

Thanks your for reply. Will delete.

> > +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).

Ok, I see. I will give the reasons.

Best regards,
Hans