linux-kernel - Re: [PATCH v2 2/4] usb: cdns2: Add main part of Cadence USBHS driver

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: <7e54e105-574a-4104-8781-2f15c0c4b329@app.fastmail.com>
Date:   Thu, 20 Apr 2023 11:43:36 +0200
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Pawel Laszczak" <pawell@...ence.com>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        Daisy.Barrera@...iusxm.com, Cliff.Holden@...iusxm.com,
        "Tony Lindgren" <tony@...mide.com>,
        "Jean Delvare" <jdelvare@...e.de>, neal_liu@...eedtech.com,
        "Linus Walleij" <linus.walleij@...aro.org>, egtvedt@...fundet.no,
        "Biju Das" <biju.das.jz@...renesas.com>, herve.codina@...tlin.com
Subject: Re: [PATCH v2 2/4] usb: cdns2: Add main part of Cadence USBHS driver

On Thu, Apr 20, 2023, at 11:00, Pawel Laszczak wrote:
> This patch introduces the main part of Cadence USBHS driver
> to Linux kernel.

Not sure why I was on Cc, but I gave it a quick look anyway, looking
for common issues. I only found a few very minor things that can be
improved, no real problems:

> +++ b/drivers/usb/gadget/udc/cdns2/Kconfig
> @@ -0,0 +1,11 @@
> +config USB_CDNS2_UDC
> +	tristate "Cadence USBHS Device Controller"
> +	depends on USB_PCI && ACPI && HAS_DMA

Why the ACPI dependency?

> +	response_pkt = (__le16 *)pdev->ep0_preq.request.buf;
> +	*response_pkt = cpu_to_le16(status);

You can simplify this using put_unaligned_le16()

> +
> +	preq->num_of_trb = num_trbs;
> +
> +	/*
> +	 * Memory barrier - cycle bit must be set as the last operation.
> +	 */
> +	wmb();

This can probably be the cheaper dma_wmb() if you only serialize
between accesses to a DMA buffer.

> +static int __maybe_unused cdns2_pci_suspend(struct device *dev)
> +{
> +	struct cdns2_device *priv_dev = dev_get_drvdata(dev);
> +
> +	return cdns2_gadget_suspend(priv_dev);
> +}
> +
> +static int __maybe_unused cdns2_pci_resume(struct device *dev)
> +{
> +	struct cdns2_device *priv_dev = dev_get_drvdata(dev);
> +
> +	return cdns2_gadget_resume(priv_dev, 1);
> +}
> +
> +static const struct dev_pm_ops cdns2_pci_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(cdns2_pci_suspend, cdns2_pci_resume)
> +};

You can use SYSTEM_SLEEP_PM_OPS() instead of SET_SYSTEM_SLEEP_PM_OPS()
and then remove the __maybe_unused.

       Arnd