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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Sep 07, 2023 at 04:06:00PM -0500, richard.yu@hpe.com wrote:
> +struct gxp_udc_drvdata {
> +	void __iomem *base;
> +	struct platform_device *pdev;
> +	struct regmap *udcg_map;
> +	struct gxp_udc_ep ep[GXP_UDC_MAX_NUM_EP];
> +
> +	int irq;
> +
> +	/* sysfs enclosure for the gadget gunk */
> +	struct device *port_dev;

A "raw" struct device?  That's not ok.  It's also going to break things,
how was this tested?  What does it look like in sysfs with this device?

> +	/*
> +	 * 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);

So you createad a "raw" device that does not belong to any bus or type
and add it to sysfs?  Why?  Shouldn't it be a "virtual" device if you
really want/need one?

> +	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");
> +		goto fail_udc;
> +	}
> +	rc = devm_request_irq(drvdata->port_dev,
> +			      drvdata->irq,
> +			      gxp_udc_irq,
> +			      IRQF_SHARED,
> +			      gxp_udc_name[drvdata->vdevnum],
> +			      drvdata);

devm_request_irq() is _very_ tricky, are you _SURE_ you got it right
here?  Why do you need to use it?

> +	if (rc < 0) {
> +		dev_err(drvdata->port_dev, "irq request failed\n");
> +		goto fail_udc;
> +	}
> +
> +	return 0;
> +
> +	/* ran code to simulate these three error exit, no double free */

What does this comment mean?

> +fail_udc:
> +	device_del(drvdata->port_dev);
> +fail_add:
> +	put_device(drvdata->port_dev);
> +fail_alloc:
> +	devm_kfree(parent, drvdata);
> +
> +	return rc;
> +}

Where is the device removed from the system when done?

thanks,

greg k-h