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

[Yu, Richard]

Hi Greg KH,

Thank you very much for the feedback and sorry for the late respond.

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?

I am using aspeed-vhub as example to write this gxp-hub driver. My struct gxp_udc_drvdata{}
Is similar to the struct ast_vhub_dev{} in drivers/usb/gadget/udc/aspeed-vhub/vhub.h
The "struct device *port_dev;" is for the child device which is attached to the hub device.

I tested this driver by modifying the create_usbhid.sh and ikvm_input.hpp in the obmc-ikvm.
The modification is just changing the dev_name to be "80400800.usb-hub". I have made sure 
that the KVM is working. (The virtual keyboard and virtual mouse are working).

The devices will be shown as 
./sys/devices/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p1
./sys/devices/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p2
./sys/devices/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p3
./sys/devices/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p4

./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p1
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p2
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p3
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p4


>> +	/*
>> +	 * 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?

I am just following the aspeed-vhub driver here. I thought I have to build the following entries:
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p1
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p2
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p3
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p4

In order for the ikvm application to get access the virtual devices.

>> +	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?

I thought this is to install my device interrupt handler. Again, I just followed the aspeed-vhub driver. The 
Aspeed-vhub driver is doing it at ast_vhub_probe() core.c file.

In previous review, Mr. Kolowski pointed out that this is very tricky using "IRQF_SHARED". I tried all the 
Available flag and none are working for me, except "IRQF_SHARED". I also confirmed that the Aspeed-vhub 
driver is also using "IRQF_SHARED".


>> +	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?

I will remove this comment. I put it in there because it was pointed out there is potential double free in
the previous review. I ran through the error exit test cases and did not see any problem.

>> +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?
I will add the device removed routine in the next check-in.

> thanks,

> greg k-h

Thank you very much

Richard.