On Thu, Sep 07, 2023 at 04:06:00PM -0500, richard.yu@xxxxxxx 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
