linux-kernel - Re: [PATCH 2/3] usb: gadget: udc: Add UDC driver for Axiado Device controller IP Corigine

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: <2026020545-headed-twirl-125c@gregkh>
Date: Thu, 5 Feb 2026 08:57:58 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Vladimir Moravcevic <vmoravcevic@...ado.com>
Cc: Krutik Shah <krutikshah@...ado.com>,
	Prasad Bolisetty <pbolisetty@...ado.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, 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 Mon, Feb 02, 2026 at 05:16:29AM -0800, 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.
> 
> The driver supports both USB 2.0 High-Speed and USB 3.0 SuperSpeed
> modes with control, bulk, interrupt, and isochronous transfer types.
> 
> Co-developed-by: Krutik Shah <krutikshah@...ado.com>
> Signed-off-by: Krutik Shah <krutikshah@...ado.com>
> Co-developed-by: Prasad Bolisetty <pbolisetty@...ado.com>
> Signed-off-by: Prasad Bolisetty <pbolisetty@...ado.com>
> Signed-off-by: Vladimir Moravcevic <vmoravcevic@...ado.com>
> ---
>  drivers/usb/gadget/udc/Kconfig   |   15 +
>  drivers/usb/gadget/udc/Makefile  |    1 +
>  drivers/usb/gadget/udc/crg_udc.c | 4522 ++++++++++++++++++++++++++++++++++++++
>  drivers/usb/gadget/udc/crg_udc.h |  364 +++
>  4 files changed, 4902 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 26460340fbc9..b94d113aad99 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -417,6 +417,21 @@ config USB_ASPEED_UDC
>  	  dynamically linked module called "aspeed_udc" and force all
>  	  gadget drivers to also be dynamically linked.
>  
> +config USB_CRG_UDC
> +	tristate "AXIADO CORIGINE-based AX3000 Device Controller"
> +	depends on ARCH_AXIADO || COMPILE_TEST
> +	depends on USB_GADGET
> +	help
> +	  Enables AX3000 USB device controller driver for Axiado
> +	  SoCs and evaluation boards.
> +
> +	  Based on the Corigine USB IP core driver with Axiado specific
> +	  enhancements. Supports USB 2.0 (High-Speed) and USB 3.0
> +	  (SuperSpeed), including control, bulk, interrupt, and
> +	  isochronous transfers.
> +
> +	  Say "y" to build statically, or "m" to build as a module.

What is the module name?  The other entries in this file all describe
this.

> --- /dev/null
> +++ b/drivers/usb/gadget/udc/crg_udc.c
> @@ -0,0 +1,4522 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

That is very odd, and I need a bit of justification as to why, and how,
MIT is allowed here.  Did you look at any of the existing gadget udc
drivers when working on this code?  If so, how can MIT still work?

Anyway, as this is a "not normal" selection for this type of driver, I
will need a signed-off-by from your corporate lawyer with the reason why
it is dual licensed described in the changelog comment for it showing
that you all understand all of the issues involved in doing something
like this, and attempting to keep it under a dual license over time.

thanks,

greg k-h

> +//
> +// Copyright (c) 2019 Corigine Inc.
> +// Copyright (c) 2022-2026 Axiado Corporation.
> +//
> +
> +#include <linux/net.h>
> +#include <asm/byteorder.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +#include <linux/unaligned.h>
> +#include <linux/types.h>
> +#include <linux/bitops.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/usb/ch9.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/delay.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/interrupt.h>
> +#include <linux/ctype.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>
> +#include <linux/kthread.h>
> +#include <linux/err.h>
> +#include <linux/wait.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/scatterlist.h>
> +#include "crg_udc.h"
> +
> +#define INIT_ZERO					-1

Why is "ZERO" defined as -1?

> +#define UDC_FALSE					false

Please just use "false" where needed.

> +
> +#define MAX_PACKET_SIZE 1024
> +
> +#define	DMA_ADDR_INVALID	(~(dma_addr_t)0)

Isn't this in the dma headers somewhere instead?  if not, why not?

And you mix tabs with spaces after the "#define" on these lists, please
don't.


> +
> +#define CRG_ERST_SIZE 1
> +#define CRG_EVENT_RING_SIZE 256

Why no tabs here, but:

> +#define CRG_NUM_EP_CX	32

Tabs here?  Be consistent please.

> +#define TRB_MAX_BUFFER_SIZE		65536
> +#define CRGUDC_CONTROL_EP_TD_RING_SIZE	16
> +#define CRGUDC_BULK_EP_TD_RING_SIZE	1024
> +#define CRGUDC_ISOC_EP_TD_RING_SIZE	32
> +#define CRGUDC_INT_EP_TD_RING_SIZE	8
> +#define CRGUDC_ROLE_DEVICE		0x1
> +
> +#define	U1_TIMEOUT_VAL	0x70
> +#define	U2_TIMEOUT_VAL	0x70

And then tabs here after "define"?

Anyway, it just stands out instantly as something odd.

> +
> +#define STATE_USB_LINK_STABLE 4
> +
> +/*********Feature switches********************/
> +#define U12_FORBIDDEN	1
> +#define U12_INITIATE_FORBIDDEN 1
> +#define CRG_UDC_INT_EN
> +#define REINIT_EP0_ON_BUS_RESET

We do not have "feature switches" in kernel drivers that require you to
rebuild the code.  Please handle these properly like all other drivers
do (i.e. not this way.)

> +/*Table 127*/

No spaces?

Anyway, what is "table 127"?  And what is it for?

> +enum TRB_CMPL_CODES_E {
> +	CMPL_CODE_INVALID       = 0,
> +	CMPL_CODE_SUCCESS,
> +	CMPL_CODE_DATA_BUFFER_ERR,
> +	CMPL_CODE_BABBLE_DETECTED_ERR,
> +	CMPL_CODE_USB_TRANS_ERR,
> +	CMPL_CODE_TRB_ERR,  /*5*/

If this really is "5", then set it to 5!


> +	CMPL_CODE_TRB_STALL,
> +	CMPL_CODE_INVALID_STREAM_TYPE_ERR = 10,
> +	CMPL_CODE_SHORT_PKT = 13,
> +	CMPL_CODE_RING_UNDERRUN,
> +	CMPL_CODE_RING_OVERRUN, /*15*/

Same here.  Don't assume that enums will be properly set without
actually setting them all, as I don't think the C standard guarantees
this (I could be wrong, but it can trip you up...)

If you want a specific value, set it so you _know_ it will be correct.

> +	CMPL_CODE_EVENT_RING_FULL_ERR = 21,
> +	CMPL_CODE_STOPPED = 26,
> +	CMPL_CODE_STOPPED_LENGTH_INVALID = 27,
> +	CMPL_CODE_ISOCH_BUFFER_OVERRUN = 31,
> +	/*192-224 vendor defined error*/

You are the vendor!

> +	CMPL_CODE_PROTOCOL_STALL = 192,
> +	CMPL_CODE_SETUP_TAG_MISMATCH = 193,
> +	CMPL_CODE_HALTED = 194,
> +	CMPL_CODE_HALTED_LENGTH_INVALID = 195,
> +	CMPL_CODE_DISABLED = 196,
> +	CMPL_CODE_DISABLED_LENGTH_INVALID = 197,
> +};
> +
> +static const char driver_name[] = "crg_udc";

Why is this needed and why not use KBUILD_MODNAME instead?

> +
> +struct buffer_info {
> +	void *vaddr;

What is a vaddr?

I'll stop here.  Also note the 0-day bot issues that it found, which
precludes us from take this as-is.

thanks,

greg k-h