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:

quoted

On Mon, Dec 27, 2021 at 05:43:16PM +0100, Niklas Schnelle wrote:

quoted

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.

quoted

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---

quoted

quoted

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.

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.

quoted

quoted

 /*
  * 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.

quoted

quoted

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)).

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.

Alan Stern