| 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
| ||
|
Message-ID: <BYAPR07MB5381A583DCBEF51CDBE2792CDD4A9@BYAPR07MB5381.namprd07.prod.outlook.com>
Date: Mon, 29 May 2023 09:37:48 +0000
From: Pawel Laszczak <pawell@...ence.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC: "Cliff.Holden@...iusxm.com" <Cliff.Holden@...iusxm.com>,
"Daisy.Barrera@...iusxm.com" <Daisy.Barrera@...iusxm.com>,
"biju.das.jz@...renesas.com" <biju.das.jz@...renesas.com>,
"egtvedt@...fundet.no" <egtvedt@...fundet.no>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"herve.codina@...tlin.com" <herve.codina@...tlin.com>,
"jdelvare@...e.de" <jdelvare@...e.de>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"neal_liu@...eedtech.com" <neal_liu@...eedtech.com>,
"tony@...mide.com" <tony@...mide.com>
Subject: RE: [PATCH v3 2/4] usb: cdns2: Add main part of Cadence USBHS driver
Hi Christophe,
You are right in all your suggestions.
I will introduce them in next version.
Thanks,
Pawel
You have
>> This patch introduces the main part of Cadence USBHS driver to Linux
>> kernel.
>> To reduce the patch size a little bit, the header file gadget.h was
>> intentionally added as separate patch.
>>
>> The Cadence USB 2.0 Controller is a highly configurable IP Core which
>> supports both full and high speed data transfer.
>>
>> The current driver has been validated with FPGA platform. We have
>> support for PCIe bus, which is used on FPGA prototyping.
>>
>> Signed-off-by: Pawel Laszczak
>> <pawell-vna1KIf7WgpBDgjK7y7TUQ@...lic.gmane.org>
>> ---
>> drivers/usb/gadget/udc/Kconfig | 2 +
>> drivers/usb/gadget/udc/Makefile | 1 +
>> drivers/usb/gadget/udc/cdns2/Kconfig | 11 +
>> drivers/usb/gadget/udc/cdns2/Makefile | 5 +
>> drivers/usb/gadget/udc/cdns2/cdns2-ep0.c | 638 +++++
>> drivers/usb/gadget/udc/cdns2/cdns2-gadget.c | 2426
>+++++++++++++++++++
>> drivers/usb/gadget/udc/cdns2/cdns2-pci.c | 149 ++
>> 7 files changed, 3232 insertions(+)
>> create mode 100644 drivers/usb/gadget/udc/cdns2/Kconfig
>> create mode 100644 drivers/usb/gadget/udc/cdns2/Makefile
>> create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-ep0.c
>> create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-gadget.c
>> create mode 100644 drivers/usb/gadget/udc/cdns2/cdns2-pci.c
>>
>> diff --git a/drivers/usb/gadget/udc/Kconfig
>> b/drivers/usb/gadget/udc/Kconfig index 83cae6bb12eb..aae1787320d4
>> 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -463,6 +463,8 @@ config USB_ASPEED_UDC
>>
>> source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig"
>>
>> +source "drivers/usb/gadget/udc/cdns2/Kconfig"
>> +
>> #
>> # LAST -- dummy/emulated controller
>> #
>> diff --git a/drivers/usb/gadget/udc/Makefile
>> b/drivers/usb/gadget/udc/Makefile index ee569f63c74a..b52f93e9c61d
>> 100644
>> --- a/drivers/usb/gadget/udc/Makefile
>> +++ b/drivers/usb/gadget/udc/Makefile
>> @@ -42,3 +42,4 @@ obj-$(CONFIG_USB_ASPEED_VHUB) += aspeed-
>vhub/
>> obj-$(CONFIG_USB_ASPEED_UDC) += aspeed_udc.o
>> obj-$(CONFIG_USB_BDC_UDC) += bdc/
>> obj-$(CONFIG_USB_MAX3420_UDC) += max3420_udc.o
>> +obj-$(CONFIG_USB_CDNS2_UDC) += cdns2/
>> diff --git a/drivers/usb/gadget/udc/cdns2/Kconfig
>> b/drivers/usb/gadget/udc/cdns2/Kconfig
>> new file mode 100644
>> index 000000000000..310db4788353
>> --- /dev/null
>> +++ 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
>> + help
>> + Cadence USBHS Device controller is a PCI based USB peripheral
>> + controller which supports both full and high speed USB 2.0
>> + data transfers.
>> +
>> + Say "y" to link the driver statically, or "m" to build a
>> + dynamically linked module called "cdns2-pci.ko" and to
>
>I'm not expert in module naming, but isn't it cdns2-udc-pci?
>
>> + force all gadget drivers to also be dynamically linked.
>
>[...]
>
>> +static void cdns2_ep_tx_isoc(struct cdns2_endpoint *pep,
>> + struct cdns2_request *preq,
>> + int num_trbs)
>> +{
>> + struct scatterlist *sg = NULL;
>> + u32 remaining_packet_size = 0;
>> + struct cdns2_trb *trb;
>> + bool first_trb = true;
>> + dma_addr_t trb_dma;
>> + u32 trb_buff_len;
>> + u32 block_length;
>> + int sg_iter = 0;
>
>Not need to init.
>
>> + int sent_len;
>> + int td_idx = 0;
>> + int split_size;
>> + u32 control;
>
>[...]
>
>> +/* Prepare and start transfer for all not started requests. */ static
>> +int cdns2_start_all_request(struct cdns2_device *pdev,
>> + struct cdns2_endpoint *pep)
>> +{
>> + struct cdns2_request *preq;
>> + int ret = 0;
>> +
>> + while (!list_empty(&pep->deferred_list)) {
>> + preq = cdns2_next_preq(&pep->deferred_list);
>> +
>> + ret = cdns2_ep_run_transfer(pep, preq);
>> + if (ret)
>> + return ret;
>> +
>> + list_move_tail(&preq->list, &pep->pending_list);
>> + }
>> +
>> + pep->ep_state &= ~EP_RING_FULL;
>> +
>> + return ret;
>
>Maybe return 0; would be more explicit? (and would remove the "= 0" above)
>
>> +}
>
>[...]
>
>> diff --git a/drivers/usb/gadget/udc/cdns2/cdns2-pci.c
>> b/drivers/usb/gadget/udc/cdns2/cdns2-pci.c
>> new file mode 100644
>> index 000000000000..ab2891c79b5c
>> --- /dev/null
>> +++ b/drivers/usb/gadget/udc/cdns2/cdns2-pci.c
>> @@ -0,0 +1,149 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Cadence USBHS-DEV controller - PCI Glue driver.
>> + *
>> + * Copyright (C) 2023 Cadence.
>> + *
>> + * Author: Pawel Laszczak
>> +<pawell-vna1KIf7WgpBDgjK7y7TUQ@...lic.gmane.org>
>> + *
>> + */
>> +
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/pci.h>
>> +
>> +#include "cdns2-gadget.h"
>> +
>> +#define PCI_DRIVER_NAME "cdns-pci-usbhs"
>> +#define CDNS_VENDOR_ID 0x17cd
>> +#define CDNS_DEVICE_ID 0x0120
>> +#define PCI_BAR_DEV 0
>> +#define PCI_DEV_FN_DEVICE 0
>> +
>> +static int cdns2_pci_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *id) {
>> + resource_size_t rsrc_start, rsrc_len;
>> + struct device *dev = &pdev->dev;
>> + struct cdns2_device *priv_dev;
>> + struct resource *res;
>> + int ret;
>> +
>> + /* For GADGET PCI (devfn) function number is 0. */
>> + if (!id || pdev->devfn != PCI_DEV_FN_DEVICE ||
>> + pdev->class != PCI_CLASS_SERIAL_USB_DEVICE)
>> + return -EINVAL;
>> +
>> + ret = pcim_enable_device(pdev);
>> + if (ret)
>> + dev_err(&pdev->dev, "Enabling PCI device has failed %d\n",
>ret);
>
>Should we bail out in this case?
>
>> +
>> + pci_set_master(pdev);
>> +
>> + priv_dev = kzalloc(sizeof(*priv_dev), GFP_KERNEL);
>> + if (!priv_dev) {
>> + ret = -ENOMEM;
>> + goto disable_pci;
>
>Any reason, not to use devm_kzalloc() and manually hanfle kfree() in the error
>handling path and in the removbe function ?
>
>> + }
>> +
>> + dev_dbg(dev, "Initialize resources\n");
>> + rsrc_start = pci_resource_start(pdev, PCI_BAR_DEV);
>> + rsrc_len = pci_resource_len(pdev, PCI_BAR_DEV);
>> +
>> + res = devm_request_mem_region(dev, rsrc_start, rsrc_len, "dev");
>> + if (!res) {
>> + dev_dbg(dev, "controller already in use\n");
>> + ret = -EBUSY;
>> + goto free_priv_dev;
>> + }
>> +
>> + priv_dev->regs = devm_ioremap(dev, rsrc_start, rsrc_len);
>> + if (!priv_dev->regs) {
>> + dev_dbg(dev, "error mapping memory\n");
>> + ret = -EFAULT;
>> + goto free_priv_dev;
>> + }
>> +
>> + priv_dev->irq = pdev->irq;
>> + dev_dbg(dev, "USBSS-DEV physical base addr: %pa\n",
>> + &rsrc_start);
>> +
>> + priv_dev->dev = dev;
>> +
>> + priv_dev->eps_supported = 0x000f000f;
>> + priv_dev->onchip_tx_buf = 16;
>> + priv_dev->onchip_rx_buf = 16;
>> +
>> + ret = cdns2_gadget_init(priv_dev);
>> + if (ret)
>> + goto free_priv_dev;
>> +
>> + pci_set_drvdata(pdev, priv_dev);
>> +
>> + device_wakeup_enable(&pdev->dev);
>> + if (pci_dev_run_wake(pdev))
>> + pm_runtime_put_noidle(&pdev->dev);
>> +
>> + return 0;
>> +
>> +free_priv_dev:
>> + kfree(priv_dev);
>> +
>> +disable_pci:
>> + pci_disable_device(pdev);
>> +
>> + return ret;
>> +}
>> +
>> +static void cdns2_pci_remove(struct pci_dev *pdev) {
>> + struct cdns2_device *priv_dev = pci_get_drvdata(pdev);
>> +
>> + if (pci_dev_run_wake(pdev))
>> + pm_runtime_get_noresume(&pdev->dev);
>> +
>> + cdns2_gadget_remove(priv_dev);
>> + kfree(priv_dev);
>
>There is a pci_disable_device() in the error handling path of the probe, but not
>in the remove function.
>
>Is it on purpose?
>Since pcim_enable_device() is used, is it needed above?
>
>CJ
>
>[...]
>
>> +static struct pci_driver cdns2_pci_driver = {
>> + .name = "cdns2-pci",
>> + .id_table = &cdns2_pci_ids[0],
>> + .probe = cdns2_pci_probe,
>> + .remove = cdns2_pci_remove,
>> + .driver = {
>> + .pm = pm_ptr(&cdns2_pci_pm_ops),
>> + }
>> +};
>> +
>> +module_pci_driver(cdns2_pci_driver);
>> +MODULE_DEVICE_TABLE(pci, cdns2_pci_ids);
>> +
>> +MODULE_ALIAS("pci:cdns2");
>> +MODULE_AUTHOR("Pawel Laszczak
>> +<pawell-vna1KIf7WgpBDgjK7y7TUQ@...lic.gmane.org>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Cadence CDNS2 PCI driver");
Powered by blists - more mailing lists