On Tue, May 16, 2023 at 06:44:34PM +0200, Arnd Bergmann wrote:
> 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.
Isn't that an abstraction? A single set of operations (uhci_readl(),
uhci_writel(), etc.) that always does the right sort of I/O even when
talking to different buses?
So I'm not sure what you mean by "the lack of abstractions".
> 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.
Indeed, in a kernel supporting that tricky combination the no-op code
would be generated. But it would never execute at runtime because the
uhci_has_pci_registers(uhci) test would always return 0, and so the
driver wouldn't fail.
> 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.
I'm sure Niklas wouldn't mind making such a change. But do we really
get such warnings? Does the compiler really think that this kind of
(macro-expanded) code:
if (uhci_has_pci_registers(uhci))
;
else if (uhci_is_aspeed(uhci))
writel(val, uhci->regs + uhci_aspeed_reg(reg));
deserves a warning? I write stuff like that fairly often; it's a good
way to showcase a high-probability do-nothing pathway at the start of a
series of conditional cases. And I haven't noticed any complaints from
the compiler.
Alan Stern
