Re: [PATCH v1 2/3] usb: gadget: udc: gxp-udc: add HPE GXP USB support

[Krzysztof Kozlowski]

On 06/07/2023 23:59, richard.yu@xxxxxxx wrote:
> From: Richard Yu <richard.yu@xxxxxxx>
> 
> The HPE GXP vEHCI controller presents a four port EHCI compatible PCI
> function to host software. Each vEHCI port is logically connected to a
> corresponding set of virtual device registers.
> 
> Signed-off-by: Richard Yu <richard.yu@xxxxxxx>
> ---
>  drivers/usb/gadget/udc/Kconfig   |    6 +
>  drivers/usb/gadget/udc/Makefile  |    1 +
>  drivers/usb/gadget/udc/gxp-udc.c | 1401 ++++++++++++++++++++++++++++++
>  3 files changed, 1408 insertions(+)
>  create mode 100644 drivers/usb/gadget/udc/gxp-udc.c
> 
> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 83cae6bb12eb..c01eb2a2c7db 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -461,6 +461,12 @@ config USB_ASPEED_UDC
>  	  dynamically linked module called "aspeed_udc" and force all
>  	  gadget drivers to also be dynamically linked.

...

> +
> +static void gxp_udc_dev_release(struct device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +int gxp_udc_init_dev(struct gxp_udcg_drvdata *udcg, unsigned int idx)
> +{
> +	struct gxp_udc_drvdata *drvdata;
> +	struct device *parent = &udcg->pdev->dev;
> +	int rc;
> +
> +	drvdata = devm_kzalloc(parent, sizeof(struct gxp_udc_drvdata),
> +			       GFP_KERNEL);

sizeof(*...)

> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	udcg->udc_drvdata[idx] = drvdata;
> +	drvdata->pdev = udcg->pdev;

Why do you store platform_device? It's usually useless because dev is used.

> +	drvdata->vdevnum = idx;
> +	drvdata->base = udcg->base + 0x1000 * idx;
> +	drvdata->udcg_map = udcg->udcg_map;
> +	drvdata->irq = udcg->irq;
> +
> +	/*
> +	 * we setup USB device can have up to 4 endpoints besides control
> +	 * endpoint 0.
> +	 */
> +	drvdata->fepnum = min_t(u32, udcg->max_fepnum, 4);
> +
> +	drvdata->vdevnum = idx;
> +
> +	/*
> +	 * The UDC core really needs us to have separate and uniquely
> +	 * named "parent" devices for each port so we create a sub device
> +	 * here for that purpose
> +	 */
> +	drvdata->port_dev = kzalloc(sizeof(*drvdata->port_dev), GFP_KERNEL);
> +	if (!drvdata->port_dev) {
> +		rc = -ENOMEM;
> +		goto fail_alloc;
> +	}
> +	device_initialize(drvdata->port_dev);
> +	drvdata->port_dev->release = gxp_udc_dev_release;
> +	drvdata->port_dev->parent = parent;
> +	dev_set_name(drvdata->port_dev, "%s:p%d", dev_name(parent), idx + 1);
> +
> +	/* DMA setting */
> +	drvdata->port_dev->dma_mask = parent->dma_mask;
> +	drvdata->port_dev->coherent_dma_mask = parent->coherent_dma_mask;
> +	drvdata->port_dev->bus_dma_limit = parent->bus_dma_limit;
> +	drvdata->port_dev->dma_range_map = parent->dma_range_map;
> +	drvdata->port_dev->dma_parms = parent->dma_parms;
> +	drvdata->port_dev->dma_pools = parent->dma_pools;
> +
> +	rc = device_add(drvdata->port_dev);
> +	if (rc)
> +		goto fail_add;
> +
> +	/* Populate gadget */
> +	gxp_udc_init(drvdata);
> +
> +	rc = usb_add_gadget_udc(drvdata->port_dev, &drvdata->gadget);
> +	if (rc != 0) {
> +		dev_err(drvdata->port_dev, "add gadget failed\n");

return dev_err_probe

> +		goto fail_udc;
> +	}
> +	rc = devm_request_irq(drvdata->port_dev,
> +			      drvdata->irq,
> +			      gxp_udc_irq,
> +			      IRQF_SHARED,
> +			      gxp_udc_name[drvdata->vdevnum],
> +			      drvdata);

Shared interrupt usually does not work with devm() and causes later
issues. You need to triple check your unbind and error paths, because
usually this just does not work. Usually we just don't allow devm for
this case. If you are sure this is 100% correct, it could stay...

> +
> +	if (rc < 0) {
> +		dev_err(drvdata->port_dev, "irq request failed\n");

return dev_err_probe

> +		goto fail_udc;
> +	}
> +
> +	return 0;
> +
> +fail_udc:
> +	device_del(drvdata->port_dev);
> +fail_add:
> +	put_device(drvdata->port_dev);
> +fail_alloc:
> +	kfree(drvdata);

Are you sure you tested it? I see here double free.

> +
> +	return rc;
> +}
> +
> +static int gxp_udcg_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	struct gxp_udcg_drvdata *drvdata;
> +	int ret, rc, err;
> +	u32 vdevnum;
> +	u32 fepnum, i;
> +
> +	drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_udcg_drvdata),

sizeof(*...)

> +			       GFP_KERNEL);
> +	platform_set_drvdata(pdev, drvdata);
> +	drvdata->pdev = pdev;

Why do you store platform_device? It's usually useless because dev is used.

> +
> +	spin_lock_init(&drvdata->lock);
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "udcg");
> +	drvdata->udcg_base = devm_ioremap_resource(&pdev->dev, res);

Use helper for these two.

> +	if (IS_ERR(drvdata->udcg_base)) {
> +		err = PTR_ERR(drvdata->udcg_base);

Drop

> +		return dev_err_probe(&pdev->dev, err, "failed to get udcg resource\n");
> +	}
> +
> +	drvdata->udcg_map = devm_regmap_init_mmio(&pdev->dev, drvdata->udcg_base, &regmap_cfg);
> +
> +	if (IS_ERR(drvdata->udcg_map)) {
> +		dev_err(&pdev->dev, "failed to map udcg regs");

return dev_err_probe

> +		return -ENODEV;

Wrong error code

> +	}
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "udc");
> +	drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(drvdata->base)) {
> +		err = PTR_ERR(drvdata->base);

Drop

> +		return dev_err_probe(&pdev->dev, err, "failed to get udc resource\n");
> +	}
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "unable to obtain IRQ number\n");

return dev_err_probe

> +		return ret;
> +	}
> +	drvdata->irq = ret;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "hpe,vehci-downstream-ports", &vdevnum)) {
> +		dev_err(&pdev->dev, "property ports number is undefined\n");
> +		return -EINVAL;
> +	}
> +
> +	if (vdevnum > GXP_UDC_MAX_NUM_VDEVICE) {
> +		dev_err(&pdev->dev, "property max port number(%d) is invalid\n", vdevnum);
> +		return -EINVAL;
> +	}
> +	drvdata->max_vdevnum = vdevnum;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "hpe,vehci-generic-endpoints", &fepnum)) {
> +		dev_err(&pdev->dev, "property generic endpoints is undefined\n");
> +		return -EINVAL;

Why? Bindings stated it has default, so should not be required.

> +	}
> +
> +	if (fepnum > GXP_UDC_MAX_NUM_FLEX_EP) {
> +		dev_err(&pdev->dev, "property fepnum(%d) is invalid\n", fepnum);
> +		return -EINVAL;
> +	}
> +	drvdata->max_fepnum = fepnum;
> +
> +	gxp_udcg_init(drvdata);
> +
> +	/* Init child devices */
> +	rc = 0;
> +	for (i = 0; i < drvdata->max_vdevnum && rc == 0; i++)
> +		rc = gxp_udc_init_dev(drvdata, i);
> +
> +	return rc;
> +}
> +
> +static const struct of_device_id gxp_udc_of_match[] = {
> +	{ .compatible = "hpe,gxp-udcg" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, gxp_udc_of_match);
> +
> +static struct platform_driver gxp_udc_driver = {
> +	.driver = {
> +		.name = "gxp-udc",
> +		.of_match_table = of_match_ptr(gxp_udc_of_match),

Drop of_match_ptr(), you will have warnings here.

> +	},
> +	.probe = gxp_udcg_probe,
> +};
> +module_platform_driver(gxp_udc_driver);
> +
> +MODULE_AUTHOR("Richard Yu <richard.yu@xxxxxxx>");
> +MODULE_DESCRIPTION("HPE GXP vEHCI UDC Driver");
> +MODULE_LICENSE("GPL");

Best regards,
Krzysztof