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] [day] [month] [year] [list]

Message-ID: <BYAPR07MB5381FF909E00820BEF3F8703DD639@BYAPR07MB5381.namprd07.prod.outlook.com>
Date:   Thu, 20 Apr 2023 10:03:57 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "Daisy.Barrera@...iusxm.com" <Daisy.Barrera@...iusxm.com>,
        "Cliff.Holden@...iusxm.com" <Cliff.Holden@...iusxm.com>,
        Tony Lindgren <tony@...mide.com>,
        Jean Delvare <jdelvare@...e.de>,
        "neal_liu@...eedtech.com" <neal_liu@...eedtech.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "egtvedt@...fundet.no" <egtvedt@...fundet.no>,
        Biju Das <biju.das.jz@...renesas.com>,
        "herve.codina@...tlin.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:

You were detected by get_maintainer.pl script :). 
I will remove you from cc in next version.

./scripts/get_maintainer.pl patch/0002-usb-cdns2-Add-main-part-of-Cadence-USBHS-driver.patch
Greg Kroah-Hartman <gregkh@...uxfoundation.org> (supporter:USB SUBSYSTEM,commit_signer:8/13=62%,commit_signer:4/6=67%)
Pawel Laszczak <pawell@...ence.com> (maintainer:CADENCE USBHS DRIVER,commit_signer:1/6=17%,added_lines:1/4=25%)
Arnd Bergmann <arnd@...db.de> (commit_signer:5/13=38%,authored:5/13=38%,added_lines:3/47=6%,removed_lines:31/47=66%,authored:1/6=17%,removed_lines:2/3=67%)
Tony Lindgren <tony@...mide.com> (commit_signer:2/13=15%)
Nicolas Ferre <nicolas.ferre@...rochip.com> (commit_signer:1/13=8%)
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org> (commit_signer:1/13=8%)
Randy Dunlap <rdunlap@...radead.org> (authored:1/13=8%)
Jean Delvare <jdelvare@...e.de> (authored:1/13=8%)
Biju Das <biju.das.jz@...renesas.com> (authored:1/13=8%,added_lines:13/47=28%,commit_signer:1/6=17%,authored:1/6=17%,added_lines:1/4=25%)
Hans-Christian Noren Egtvedt <egtvedt@...fundet.no> (authored:1/13=8%)
Neal Liu <neal_liu@...eedtech.com> (added_lines:13/47=28%,commit_signer:1/6=17%,authored:1/6=17%,added_lines:1/4=25%)
Herve Codina <herve.codina@...tlin.com> (added_lines:11/47=23%,commit_signer:1/6=17%,authored:1/6=17%,added_lines:1/4=25%)
Linus Walleij <linus.walleij@...aro.org> (removed_lines:11/47=23%,authored:1/6=17%,removed_lines:1/3=33%)
linux-kernel@...r.kernel.org (open list)

Thanks,
Pawel

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