Re: [PATCH v5 2/3] PCI: Move pci_bus_speed2lnkctl2() to public header

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4136 bytes --]

On Tue, 7 Apr 2026, Hans Zhang wrote:
> On 4/7/26 20:29, Hans Zhang wrote:
> > On 4/7/26 20:26, Ilpo Järvinen wrote:
> > > On Tue, 7 Apr 2026, Hans Zhang wrote:
> > > > On 4/7/26 16:11, Ilpo Järvinen wrote:
> > > > > On Mon, 6 Apr 2026, Hans Zhang wrote:
> > > > > 
> > > > > > Move the static array-based pci_bus_speed2lnkctl2() function from
> > > > > > bwctrl.c to pci.h as a public inline function.
> > > > > > 
> > > > > > This provides efficient O(1) speed-to-LNKCTL2 value conversion using
> > > > > > static array lookup, maintaining optimal performance while enabling
> > > > > > code reuse by other PCIe drivers.
> > > > > > 
> > > > > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > > > > ---
> > > > > >    drivers/pci/pci.h         | 17 +++++++++++++++++
> > > > > >    drivers/pci/pcie/bwctrl.c | 17 -----------------
> > > > > >    2 files changed, 17 insertions(+), 17 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > > > > index f0a082bfd6f1..db91878a86ac 100644
> > > > > > --- a/drivers/pci/pci.h
> > > > > > +++ b/drivers/pci/pci.h
> > > > > > @@ -611,6 +611,23 @@ static inline bool pcie_valid_speed(enum
> > > > > > pci_bus_speed speed)
> > > > > >        return (speed >= PCIE_SPEED_2_5GT) && (speed <=
> > > > > > PCIE_SPEED_64_0GT);
> > > > > >    }
> > > > > >    +static inline u16 pci_bus_speed2lnkctl2(enum pci_bus_speed
> > > > > > speed)
> > > > > > +{
> > > > > > +    static const u8 speed_conv[] = {
> > > > > > +        [PCIE_SPEED_2_5GT] = PCI_EXP_LNKCTL2_TLS_2_5GT,
> > > > > > +        [PCIE_SPEED_5_0GT] = PCI_EXP_LNKCTL2_TLS_5_0GT,
> > > > > > +        [PCIE_SPEED_8_0GT] = PCI_EXP_LNKCTL2_TLS_8_0GT,
> > > > > > +        [PCIE_SPEED_16_0GT] = PCI_EXP_LNKCTL2_TLS_16_0GT,
> > > > > > +        [PCIE_SPEED_32_0GT] = PCI_EXP_LNKCTL2_TLS_32_0GT,
> > > > > > +        [PCIE_SPEED_64_0GT] = PCI_EXP_LNKCTL2_TLS_64_0GT,
> > > > > > +    };
> > > > > > +
> > > > > > +    if (WARN_ON_ONCE(!pcie_valid_speed(speed)))
> > > > > 
> > > > > drivers/pci/pci.h doesn't seem to have include for WARN_ON_ONCE() so
> > > > > you
> > > > > should add it.
> > > > 
> > > > Hi Ilpo,
> > > > 
> > > > In the file "drivers/pci/pcie/bwctrl.c", there is no reference to the
> > > > header
> > > > file of WARN_ON_ONCE(). It seems that no error was reported. I think it
> > > > might
> > > > be that some other header files have indirectly referenced the header
> > > > file of
> > > > WARN_ON_ONCE(). However, when I compiled it locally, no errors were
> > > > reported.
> > > 
> > > Hi,
> > > 
> > > Apparently it was missing from there as well.
> > > 
> > > It might build now (I don't actually even doubt that), but that depends
> > > on the includes in every file including drivers/pci/pci.h. We should not
> > > depend on that as it is fragile but ensure a header is self-sufficient
> > > when it comes to includes necessary for it.
> > 
> > Hi Ilpo,
> > 
> > OK, I see.
> > 
> > 
> > > 
> > > > I think if we want to add a header file, it should be #include
> > > > <asm-generic/bug.h> ?
> > > 
> > > Yes
> > > 
> > > #include <asm/bug.h>
> 
> Hi Ilpo,
> 
> Executing the following check script will trigger a warning. However, using
> #include <asm-generic/bug.h> will not cause any issues. So, should we use
> #include <asm-generic/bug.h> ?
> 
> ./scripts/checkpatch.pl ...../*patch
> 
> 0002-PCI-Move-pci_bus_speed2lnkctl2-to-public-header.patch
> ---------------------------------------------------------------------
> WARNING: Use #include <linux/bug.h> instead of <asm/bug.h>
> #27: FILE: drivers/pci/pci.h:5:
> +#include <asm/bug.h>
> 
> total: 0 errors, 1 warnings, 53 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.

This is a header, so using <asm/bug.h> minimizes what it pulls in so it 
is justified to use asm/ over linux/ in this case.


-- 
 i.