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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <SJ0PR84MB20855ADDEE46D6A437D8E7C18DDEA@SJ0PR84MB2085.NAMPRD84.PROD.OUTLOOK.COM>
Date:   Wed, 25 Oct 2023 22:14:49 +0000
From:   "Yu, Richard" <richard.yu@....com>
To:     Greg KH <gregkh@...uxfoundation.org>
CC:     "Verdun, Jean-Marie" <verdun@....com>,
        "Hawkins, Nick" <nick.hawkins@....com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 2/3] usb: gadget: udc: gxp-udc: add HPE GXP USB HUB
 support

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@....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?

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@...00000/80400800.usb-hub/80400800.usb-hub:p1
./sys/devices/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p2
./sys/devices/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p3
./sys/devices/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p4

./sys/bus/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p1
./sys/bus/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p2
./sys/bus/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p3
./sys/bus/platform/devices/ahb@...00000/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@...00000/80400800.usb-hub/80400800.usb-hub:p1
./sys/bus/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p2
./sys/bus/platform/devices/ahb@...00000/80400800.usb-hub/80400800.usb-hub:p3
./sys/bus/platform/devices/ahb@...00000/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.