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:
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>
---
drivers/usb/core/hcd-pci.c | 3 +-
drivers/usb/host/Kconfig | 4 +-
drivers/usb/host/pci-quirks.c | 127 ++++++++++++++++++----------------
drivers/usb/host/pci-quirks.h | 33 ++++++---
drivers/usb/host/uhci-hcd.c | 2 +-
drivers/usb/host/uhci-hcd.h | 77 ++++++++++++++-------
6 files changed, 148 insertions(+), 98 deletions(-)
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.
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.
quoted
else if (pdev->class == PCI_CLASS_SERIAL_USB_OHCI)
quirk_usb_handoff_ohci(pdev);
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index e729de21fad7..42eb18be37af 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -2,33 +2,50 @@
#ifndef __LINUX_USB_PCI_QUIRKS_H
#define __LINUX_USB_PCI_QUIRKS_H
-#ifdef CONFIG_USB_PCI
void uhci_reset_hc(struct pci_dev *pdev, unsigned long base);
int uhci_check_and_reset_hc(struct pci_dev *pdev, unsigned long base);
-int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *pdev);
+
+struct pci_dev;
This can't be right; struct pci_dev is referred to three lines earlier.
You could move this up, but it may not be needed at all.
I agree.
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.