linux-kernel - Re: [PATCH v3 34/38] 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: <9e89c4af-7682-4d30-93e7-703fde28180f@app.fastmail.com>
Date:   Tue, 14 Mar 2023 15:56:56 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Niklas Schnelle" <schnelle@...ux.ibm.com>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Mathias Nyman" <mathias.nyman@...el.com>,
        "Alan Stern" <stern@...land.harvard.edu>
Cc:     "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 v3 34/38] usb: handle HAS_IOPORT dependencies

On Tue, Mar 14, 2023, at 13:12, 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.
>
> Co-developed-by: Arnd Bergmann <arnd@...nel.org>
> Signed-off-by: Niklas Schnelle <schnelle@...ux.ibm.com>

I would suggest splitting this patch up into separate bits for the AMD
quirks and the UHCI driver, possibly more if there are other parts
unrelated to that.

> +#ifdef CONFIG_HAS_IOPORT
>  /*
>   * Make sure the controller is completely inactive, unable to
>   * generate interrupts or do DMA.

Maybe check for both HAS_IOPORT and USB_UHCI_HCD ?

>  static inline int io_type_enabled(struct pci_dev *pdev, unsigned int 
> mask)
>  {
> @@ -725,6 +730,7 @@ static inline int io_type_enabled(struct pci_dev 
> *pdev, unsigned int mask)
> 
>  static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
>  {
> +#ifdef HAS_IOPORT
>  	unsigned long base = 0;
>  	int i;
> 
> @@ -739,6 +745,7 @@ static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
> 
>  	if (base)
>  		uhci_check_and_reset_hc(pdev, base);
> +#endif
>  }

> diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> index 0688c3e5bfe2..c77705d03ed0 100644
> --- a/drivers/usb/host/uhci-hcd.h
> +++ b/drivers/usb/host/uhci-hcd.h
> @@ -505,41 +505,49 @@ static inline bool uhci_is_aspeed(const struct 
> uhci_hcd *uhci)
>   * we use memory mapped registers.
>   */
> 
> +#ifdef CONFIG_HAS_IOPORT
> +#define UHCI_IN(x)	x
> +#define UHCI_OUT(x)	x
> +#else
> +#define UHCI_IN(x)	0
> +#define UHCI_OUT(x)
> +#endif
> +
>  #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));
>  }
> 

This looks a bit ugly, though I can't think of a version I
really like here though. Possibly merging this together with the
generic version would result in something better, like

#if defined(CONFIG_USB_UHCI_PCI) && defined(CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC)
/* Support PCI and non-PCI host controllers */
#define uhci_has_pci_registers(u)       ((u)->io_addr != 0)
#elif defined(CONFIG_USB_UHCI_PCI)
#define uhci_has_pci_registers(u)       1
#else           
/* Support non-PCI host controllers only */
#define uhci_has_pci_registers(u)       0
#endif 

static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
{
#ifdef CONfIG_USB_UHCI_PCI
        if (uhci_has_pci_registers(uhci))
                return inl(uhci->io_addr + reg);
        else
#endif
        if (uhci_is_aspeed(uhci))
                return readl(uhci->regs + uhci_aspeed_reg(reg));
        else
#ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
        if (uhci_big_endian_mmio(uhci))
                return readl_be(uhci->regs + reg);
        else
#endif
        return readl(uhci->regs + reg);
}

Obviously still ugly, not sure if anyone can come up with a better
version.

      Arnd