Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Alan Stern]

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