On Tue, May 16, 2023 at 06:29:56PM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > not being declared. We thus need to guard sections of code calling them
> > as alternative access methods with CONFIG_HAS_IOPORT checks. For
> > uhci-hcd there are a lot of I/O port uses that do have MMIO alternatives
> > all selected by uhci_has_pci_registers() so this can be handled by
> > UHCI_IN/OUT macros and making uhci_has_pci_registers() constant 0 if
> > CONFIG_HAS_IOPORT is unset.
> >
> > Co-developed-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > ---
> > Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
> > per-subsystem patches may be applied independently
> >
> > drivers/usb/host/uhci-hcd.c | 2 +-
> > drivers/usb/host/uhci-hcd.h | 36 +++++++++++++++++++++++-------------
> > 2 files changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> > index 7cdc2fa7c28f..fd2408b553cf 100644
> > --- a/drivers/usb/host/uhci-hcd.c
> > +++ b/drivers/usb/host/uhci-hcd.c
> > @@ -841,7 +841,7 @@ static int uhci_count_ports(struct usb_hcd *hcd)
> >
> > static const char hcd_name[] = "uhci_hcd";
> >
> > -#ifdef CONFIG_USB_PCI
> > +#if defined(CONFIG_USB_PCI) && defined(CONFIG_HAS_IOPORT)
> > #include "uhci-pci.c"
> > #define PCI_DRIVER uhci_pci_driver
> > #endif
> > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > index 0688c3e5bfe2..c77705d03ed0 100644
> > --- a/drivers/usb/host/uhci-hcd.h
> > +++ b/drivers/usb/host/uhci-hcd.h
> > @@ -505,41 +505,49 @@ static inline bool uhci_is_aspeed(const struct uhci_hcd *uhci)
> > * we use memory mapped registers.
> > */
> >
> > +#ifdef CONFIG_HAS_IOPORT
> > +#define UHCI_IN(x) x
> > +#define UHCI_OUT(x) x
> > +#else
> > +#define UHCI_IN(x) 0
> > +#define UHCI_OUT(x)
> > +#endif
> > +
> > #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> > /* Support PCI only */
> > static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> > {
> > - return inl(uhci->io_addr + reg);
> > + return UHCI_IN(inl(uhci->io_addr + reg));
> > }
> >
> > static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
> > {
> > - outl(val, uhci->io_addr + reg);
> > + UHCI_OUT(outl(val, uhci->io_addr + reg));
>
> I'm confused now.
>
> So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
>
> But if it isn't, then these are just no-ops that do nothing? So then
> the driver will fail to work? Why have these stubs at all?
>
> Why not just not build the driver at all if this option is not enabled?
I should add something to my previous email. This particular section of
code is protected by:
#ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
/* Support PCI only */
So it gets used only in cases where the driver supports just a PCI bus
-- no other sorts of non-PCI on-chip devices. But the preceding patch
in this series changes the Kconfig file to say:
config USB_UHCI_HCD
tristate "UHCI HCD (most Intel and VIA) support"
depends on (USB_PCI && HAS_IOPORT) || USB_UHCI_SUPPORT_NON_PCI_HC
As a result, when the configuration includes support only for PCI
controllers the driver won't get built unless HAS_IOPORT is set. Thus
the no-op case (in this part of the code) can't arise.
Which is a long-winded way of saying that you're right; the UHCI_IN()
and UHCI_OUT() wrappers aren't needed in this part of the driver. I
guess Niklas put them in either for consistency with the rest of the
code or because it didn't occur to him that they could be omitted. (And
I didn't spot it either.)
Alan Stern
