linux-kernel - Re: [PATCH v4 35/41] usb: uhci: handle HAS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <23936929-80e4-4599-827a-d09b4960f3ab@rowland.harvard.edu>
Date:   Tue, 16 May 2023 15:51:30 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Niklas Schnelle <schnelle@...ux.ibm.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-kernel@...r.kernel.org,
        Linux-Arch <linux-arch@...r.kernel.org>,
        linux-pci@...r.kernel.org, Arnd Bergmann <arnd@...nel.org>,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

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