linux-kernel - Re: [RFC 31/32] usb: 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: <8bda347ea30b60f1edb55693ff7509e7f7b1f979.camel@linux.ibm.com>
Date:   Fri, 31 Dec 2021 12:06:24 +0100
From:   Niklas Schnelle <schnelle@...ux.ibm.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Arnd Bergmann <arnd@...nel.org>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        John Garry <john.garry@...wei.com>,
        Nick Hu <nickhu@...estech.com>,
        Greentime Hu <green.hu@...il.com>,
        Vincent Chen <deanbo422@...il.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>, Guo Ren <guoren@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
        linux-csky@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [RFC 31/32] usb: handle HAS_IOPORT dependencies

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:
> > 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.
> 
> > Co-developed-by: Arnd Bergmann <arnd@...nel.org>
> > Signed-off-by: Arnd Bergmann <arnd@...nel.org>
> > Signed-off-by: Niklas Schnelle <schnelle@...ux.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.

> 
> >  /*
> >   * 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.

> 
> >  	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.

> 
> > 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.