On Tue, May 16, 2023, at 18:29, Greg Kroah-Hartman wrote: > On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote: >> #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? If I remember correctly, the problem here is the lack of abstractions in the uhci driver, it instead supports all combinations of on-chip non-PCI devices using readb()/writeb() and PCI devices using inb()/outb() in a shared codebase. A particularly tricky combination is a kernel that supports on-chip UHCI as well as CONFIG_USB_PCI (for EHCI/XHCI) but does not support I/O ports because of platform limitations. The trick is to come up with a set of changes that doesn't have to rewrite the entire logic but also doesn't add an obscene number of #ifdef checks. That said, there is a minor problem with the empty definition +#define UHCI_OUT(x) I think this should be "do { } while (0)" to avoid warnings about empty if/else blocks. Arnd