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.
