Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Arnd Bergmann]

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