Re: [RFC 31/32] usb: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Fri, 2021-12-31 at 12:15 -0500, Alan Stern wrote:
> On Fri, Dec 31, 2021 at 12:06:24PM +0100, Niklas Schnelle wrote:
> > On Mon, 2021-12-27 at 15:36 -0500, Alan Stern wrote:
> > > On Mon, Dec 27, 2021 at 05:43:16PM +0100, 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. Similarly
> > > > drivers requiring these functions need to depend on HAS_IOPORT.
> > > 
> > > A few things in here can be improved.
> > > 
> > > > Co-developed-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > > > Signed-off-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> > > > Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> > > > ---
> > > > diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> > > > index ef08d68b9714..bba320194027 100644
> > > > --- a/drivers/usb/host/pci-quirks.c
> > > > +++ b/drivers/usb/host/pci-quirks.c
> > > > +#ifdef CONFIG_USB_PCI_AMD
> > > > +#if IS_ENABLED(CONFIG_USB_UHCI_HCD) && defined(CONFIG_HAS_IOPORT)
> > > 
> > > In the original, the following code will be compiled even if
> > > CONFIG_USB_UHCI_HCD is not enabled.  You shouldn't change that.
> > 
> > If this was only '#ifdef CONFIG_HAS_IOPORT' we would leave
> > uhci_reset_hc() undeclared in the case where CONFIG_HAS_IOPORT is
> > unset. This function however is also called from uhci-pci.c. That on
> > the other hand is built only if CONFIG_USB_UHCI_HCD is set so if we
> > depend on both config options we can get rid of all calls and have the
> > functions undeclared.
> 
> But you changed the guard around the '#include "uhci-pci.c"' line in 
> uhci-hcd.c, so uhci-pci.c won't be built if CONFIG_HAS_IOPORT is unset.  
> Thus there won't be an undefined function call regardless.

Ah thanks yes that makes sense. I seem to have gotten confused by the
uhci-hcd.c vs uhci-pci.c dependency.

> 
> You see, even if the kernel isn't configured to include a UHCI driver, 
> it's still important to hand off and disable any PCI UHCI hardware when 
> the system starts up.  Otherwise you can get all sorts of crazy 
> interrupts and DMA from the BIOS.

Thanks for the explanation, this is very helpful to understand the
context.

> 
> > > >  /*
> > > >   * Make sure the controller is completely inactive, unable to
> > > >   * generate interrupts or do DMA.
> > > > @@ -1273,7 +1277,8 @@ static void quirk_usb_early_handoff(struct pci_dev *pdev)
> > > >  			 "Can't enable PCI device, BIOS handoff failed.\n");
> > > >  		return;
> > > >  	}
> > > > -	if (pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
> > > > +	if (IS_ENABLED(CONFIG_USB_UHCI_HCD) &&
> > > > +	    pdev->class == PCI_CLASS_SERIAL_USB_UHCI)
> > > >  		quirk_usb_handoff_uhci(pdev);
> > > 
> > > Same idea here.
> > 
> > Hmm, I'm not 100% sure if the IS_ENABLED(CONFIG_USB_UHCI_HCD) depends
> > on some compiler optimizations for it to be ok that
> > uhci_check_and_reset_hc() is not declared in the case where both
> > CONFIG_HAS_IOPORT and CONFIG_USB_UHCI_HCD are unset. Maybe that should
> > be a plain ifdef.
> 
> The reasoning should be exactly the same as in the previous case.

Yeah with your above explanation it makes sense that we need to keep
the calls even if CONFIG_USB_UHCI_HCD is not enabled. We're also
handling the CONFIG_HAS_IOPORT=n case in quirk_usb_handoff_uhci()
anyway.

> 
> > > > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > > > index 8ae5ccd26753..8e30116b6fd2 100644
> > > > --- a/drivers/usb/host/uhci-hcd.h
> > > > +++ b/drivers/usb/host/uhci-hcd.h
> > > > @@ -586,12 +586,14 @@ static inline int uhci_aspeed_reg(unsigned int reg)
> > > >  
> > > >  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> > > >  {
> > > > +#ifdef CONFIG_HAS_IOPORT
> > > >  	if (uhci_has_pci_registers(uhci))
> > > >  		return inl(uhci->io_addr + reg);
> > > > -	else if (uhci_is_aspeed(uhci))
> > > > +#endif
> > > 
> > > Instead of making all these changes (here and in the hunks below), you
> > > can simply modify the definition of uhci_has_pci_registers() so that it
> > > always gives 0 when CONFIG_HAS_IOPORT is N.
> > > 
> > > Alan Stern
> > 
> > I don't think that works, for example in the hunk you quoted returning
> > 0 from uhci_has_pci_registers() only skips over the inl() at run-time.
> > We're aiming to have inl() undeclared if HAS_IOPORT is unset though.
> 
> I see.  Do you think the following would be acceptable?  Add:
> 
> #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
> 
> and then replace for example inl(uhci->io_addr + reg) with 
> UHCI_IN(inl(uhci->io_addr + reg)).

In principle that looks like a valid approach. Not sure this is better
than explicit ifdefs though. With this approach one could add
UHCI_IN()/UHCI_OUT() calls which end up as nops without realizing it as
it would disable any compile time warning for using them without
guarding against CONFIG_HAS_IOPORT being undefined.

> 
> The definition of uhci_has_pci_registers() should be updated in any 
> case; there's no reason for it to do a runtime check of uhci->io_addr 
> when HAS_IOPORT is disabled.

Agree. Interestingly same as with the "if
(IS_ENABLED(CONFIG_HAS_IOPORT))" it seems having
uhci_has_pci_registers() compile-time defined to 0 (I added a
defined(CONFIG_HAS_IOPORT) to it) makes the compiler ignore the missing
inl() decleration already. But I'm not sure if we should rely on that.