| 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: <82f7f4f4-6f88-4293-ae13-5da9d7252efa@kernel.org>
Date: Thu, 5 Feb 2026 08:46:46 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Vladimir Moravcevic <vmoravcevic@...ado.com>,
Krutik Shah <krutikshah@...ado.com>, Prasad Bolisetty
<pbolisetty@...ado.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
openbmc@...ts.ozlabs.org
Subject: Re: [PATCH 2/3] usb: gadget: udc: Add UDC driver for Axiado Device
controller IP Corigine
On 02/02/2026 14:16, Vladimir Moravcevic wrote:
> Add Corigine USB IP Driver for Axiado AX3000 SoC's
> USB peripheral (USB 2.0/3.0).
> The driver is based on the Corigine USB IP core with
> Axiado-specific enhancements including VBUS detection and USB link
> stability fixes.
This driver looks way too complicated for simple USB controller, so I
guess you just re-implemented a lot of Linux stack or other drivers.
Also did not pass basic litmus test for sending usu 15 year old junk
code, which disqualifies it from review IMO. There is simply no point to
review code from 15 yaers ago - you should never start with such code.
> +static const struct crg_udc_priv ax3000_plat_setup_gen2 = {
> + .plat_setup_gen3 = false,
> +};
> +
> +static const struct crg_udc_priv ax3000_plat_setup_gen3 = {
> + .plat_setup_gen3 = true,
> +};
> +
> +/**
> + * crg_gadget_probe - Initializes gadget driver
> + *
> + *
> + * Returns 0 on success otherwise negative errno.
> + */
Completely pointless and wrongly placed comment. Do not ever add such
comments.
> +
> +static const struct of_device_id of_crg_udc_match[] = {
> + {
> + .compatible = "axiado,ax3000-udc",
> + .data = &ax3000_plat_setup_gen2
> + },
> + {
> + .compatible = "axiado,ax3000-udc-gen3",
> + .data = &ax3000_plat_setup_gen3
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, of_crg_udc_match);
> +
> +static int crg_udc_probe(struct platform_device *pdev)
> +{
> + int ret;
> + int i;
> + struct crg_gadget_dev *crg_udc;
> + static int udc_gcnt = INIT_ZERO;
> + char udc_gname[10] = {""};
> + const struct crg_udc_priv *priv;
> +
> + priv = of_device_get_match_data(&pdev->dev);
> + sprintf(udc_gname, "gadget-%d", udc_gcnt);
> + crg_udc = devm_kzalloc(&pdev->dev, sizeof(*crg_udc), GFP_KERNEL);
> + if (!crg_udc)
> + return -ENOMEM;
> + crg_udc->dev = &pdev->dev;
> +
> + spin_lock_init(&crg_udc->udc_lock);
> + platform_set_drvdata(pdev, crg_udc);
> +
> + dev_set_name(&crg_udc->gadget.dev, udc_gname);
> + crg_udc->gadget.ops = &crg_gadget_ops;
> + crg_udc->gadget.ep0 = &crg_udc->udc_ep[0].usb_ep;
> + crg_udc->gadget.dev.parent = &pdev->dev;
> + INIT_LIST_HEAD(&crg_udc->gadget.ep_list);
> + if (priv->plat_setup_gen3) {
> + crg_udc->gadget.max_speed = USB_SPEED_SUPER;
> + crg_udc->gadget.speed = USB_SPEED_SUPER;
> + } else {
> + crg_udc->gadget.max_speed = USB_SPEED_HIGH;
> + crg_udc->gadget.speed = USB_SPEED_HIGH;
> + }
> + crg_udc->gadget.name = udc_gname;
> + crg_udc->gadget.sg_supported = true;
> + dev_dbg(crg_udc->dev, "%s sg support\n", __func__);
> + crg_udc->connected = 0;
> + crg_udc->dev_addr = 0;
> +
> + crg_udc->udc_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!crg_udc->udc_res) {
> + dev_err(&pdev->dev, "missing memory resource\n");
> + return -ENODEV;
> + }
> +
> + crg_udc->mmio_virt_base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(crg_udc->mmio_virt_base)) {
> + dev_err(&pdev->dev, "mmio ioremap failed\n");
> + return PTR_ERR(crg_udc->mmio_virt_base);
> + }
> +
> + /* set controller device role*/
> + writel((readl(crg_udc->mmio_virt_base + CRG_UDC_MODE_REG) |
> + CRGUDC_ROLE_DEVICE),
> + crg_udc->mmio_virt_base + CRG_UDC_MODE_REG);
> + for (i = 0; i < CRG_RING_NUM; i++) {
> + crg_udc->uicr[i] = crg_udc->mmio_virt_base +
> + CRG_UICR_OFFSET + i * CRG_UICR_STRIDE;
> +
> + dev_dbg(crg_udc->dev, "crg_udc->uicr[%d] = %p\n", i,
> + crg_udc->uicr[i]);
> + }
> + crg_udc->uccr = crg_udc->mmio_virt_base + CRG_UCCR_OFFSET;
> +
> + crg_udc_reset(crg_udc);
> +
> + crg_udc_clear_portpm(crg_udc);
> +
> + ret = reset_data_struct(crg_udc);
> + if (ret) {
> + dev_err(crg_udc->dev, "reset_data_struct error\n");
> + goto err0;
> + }
> +
> + init_ep_info(crg_udc);
> + init_ep0(crg_udc);
> +
> + EP0_Start(crg_udc);
Did you read coding style?
> +
> + crg_gadget_irq_init(pdev, crg_udc);
> +
> + ret = usb_add_gadget_udc(&pdev->dev, &crg_udc->gadget);
> + if (ret)
> + goto err0;
> +
> + udc_gcnt++;
> +
> + return 0;
> +
> +err0:
> + return -1;
What?
> +}
> +
> +static void crg_udc_remove(struct platform_device *pdev)
> +{
> + struct crg_gadget_dev *crg_udc = platform_get_drvdata(pdev);
> + u32 tmp = 0;
> +
> + dev_dbg(crg_udc->dev, "%s %d called\n", __func__, __LINE__);
> +
> + crg_udc->device_state = USB_STATE_ATTACHED;
> + crg_vbus_detect(crg_udc, 0);
> +
> + usb_del_gadget_udc(&crg_udc->gadget);
> +
> + /* set controller host role*/
> + tmp = readl(crg_udc->mmio_virt_base + CRG_UDC_MODE_REG) & ~0x1;
> + writel(tmp, crg_udc->mmio_virt_base + CRG_UDC_MODE_REG);
> +
> + if (crg_udc->irq)
> + free_irq(crg_udc->irq, crg_udc);
> +
> + platform_set_drvdata(pdev, 0);
> +
> + dev_dbg(crg_udc->dev, "%s %d gadget remove\n", __func__, __LINE__);
Drop all such debugs.
> +
> +}
> +
> +static void crg_udc_shutdown(struct platform_device *pdev)
> +{
> + struct crg_gadget_dev *crg_udc = platform_get_drvdata(pdev);
> +
> + dev_dbg(crg_udc->dev, "%s %d called\n", __func__, __LINE__);
It's really pointless code.
> +
> + crg_udc->device_state = USB_STATE_ATTACHED;
> + crg_vbus_detect(crg_udc, 0);
> + usb_del_gadget_udc(&crg_udc->gadget);
> +
> + if (crg_udc->irq)
> + free_irq(crg_udc->irq, crg_udc);
> + /*
> + * Clear the drvdata pointer.
> + */
> + platform_set_drvdata(pdev, 0);
> +}
> +
> +#ifdef CONFIG_PM
> +static int crg_udc_suspend(struct device *dev)
> +{
> + return 0;
> +}
> +
> +static int crg_udc_resume(struct device *dev)
> +{
> +
> +
> + return 0;
> +}
> +#else
> +#define crg_udc_suspend NULL
> +#define crg_udc_resume NULL
> +#endif
> +
> +static const struct dev_pm_ops crg_udc_pm_ops = {
> + .suspend = crg_udc_suspend,
> + .resume = crg_udc_resume,
> +};
> +
> +static struct platform_driver crg_udc_driver = {
> + .probe = crg_udc_probe,
> + .remove = crg_udc_remove,
> + .shutdown = crg_udc_shutdown,
> + .driver = {
> + .name = "crg_udc",
> + .owner = THIS_MODULE,
Do not upstream 10 or 15 year old driver. Why do we need to repeat all
the same comments as we repeated for last 15 years? Take newest driver
as starting point, not 15 year old code. You just replicated all old issues.
Best regards,
Krzysztof
Powered by blists - more mailing lists