Re: [v7 1/5] PCI: Refactor capability search into common macros

[Hans Zhang]

On 2025/4/3 17:10, Ilpo Järvinen wrote:
> > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > index 2e9cf26a9ee9..f705b8bd3084 100644
> > > > --- a/drivers/pci/pci.h
> > > > +++ b/drivers/pci/pci.h
> > > > @@ -89,6 +89,87 @@ 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);
> > > >    +/* Standard Capability finder */
> > > > +/**
> > > > + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
> > > > + * @read_cfg: Function pointer for reading PCI config space
> > > > + * @start: Starting position to begin search
> > > > + * @cap: Capability ID to find
> > > > + * @args: Arguments to pass to read_cfg function
> > > > + *
> > > > + * Iterates through the capability list in PCI config space to find
> > > > + * the specified capability. Implements TTL (time-to-live) protection
> > > > + * against infinite loops.
> > > > + *
> > > > + * Returns: Position of the capability if found, 0 otherwise.
> > > > + */
> > > > +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)
> > > > \
> > > > +({									\
> > > > +	u8 __pos = (start);						\
> > > > +	int __ttl = PCI_FIND_CAP_TTL;					\
> > > > +	u16 __ent;							\
> > > > +	u8 __found_pos = 0;						\
> > > > +	u8 __id;							\
> > > > +									\
> > > > +	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
> > > > +									\
> > > > +	while (__ttl--) {						\
> > > > +		if (__pos < PCI_STD_HEADER_SIZEOF)			\
> > > > +			break;						\
> > > > +		__pos = ALIGN_DOWN(__pos, 4);				\
> > > > +		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
> > > > +		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
> > > > +		if (__id == 0xff)					\
> > > > +			break;						\
> > > > +		if (__id == (cap)) {					\
> > > > +			__found_pos = __pos;				\
> > > > +			break;						\
> > > > +		}							\
> > > > +		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
> > >
> > > Could you please separate the coding style cleanups into own patch that
> > > is before the actual move patch. IMO, all those cleanups can be in the
> > > same patch.
> >
> > Hi Ilpo,
> >
> > Thanks your for reply. I don't understand. Is it like this?
>
> Add a patch before the first patch which does only the cleanups to
> __pci_find_next_cap_ttl(). The patch that creates PCI_FIND_NEXT_CAP_TTL()
> and converts its PCI core users (most of the patches 1&2) is to be based
> on top of that cleanup patch.

Hi Ilpo,

Thank you so much for your patience in explaining it to me.

> > #define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
> > ({									\
> > 	int __ttl = PCI_FIND_CAP_TTL;					\
> > 	u8 __id, __found_pos = 0;					\
> > 	u8 __pos = (start);						\
> > 	u16 __ent;							\
> > 									\
> > 	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
> > 									\
> > 	while (__ttl--) {						\
> > 		if (__pos < PCI_STD_HEADER_SIZEOF)			\
> > 			break;						\
> > 									\
> > 		__pos = ALIGN_DOWN(__pos, 4);				\
> > 		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
> > 									\
> > 		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
> > 		if (__id == 0xff)					\
> > 			break;						\
> > 									\
> > 		if (__id == (cap)) {					\
> > 			__found_pos = __pos;				\
> > 			break;						\
> > 		}							\
> > 									\
> > 		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
> > 	}								\
> > 	__found_pos;							\
> > })
> >
> > > You also need to add #includes for the defines you now started to use.
> >
> > Is that what you mean?
> >
> > +#include <linux/bitfield.h>
> > +#include <linux/align.h>
> > +#include <uapi/linux/pci_regs.h>
>
> Almost, including pci_regs.h is not strictly necessary as linux/pci.h will
> always pull that one in (not that it would hurt).
>
> Also, sort the includes alphabetically.

OK，will change.

Best regards,
Hans