Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   "Yu, Richard" <richard.yu@hpe.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     "Verdun, Jean-Marie" <verdun@hpe.com>,
        "Hawkins, Nick" <nick.hawkins@hpe.com>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "conor+dt@kernel.org" <conor+dt@kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 2/3] usb: gadget: udc: gxp-udc: add HPE GXP USB HUB
 support
Thread-Topic: [PATCH v2 2/3] usb: gadget: udc: gxp-udc: add HPE GXP USB HUB
 support
Thread-Index: AQHZ4c+k4sD8rkOE10yCm8eHVE9HNrA2kkCAgBy9EGA=
Date:   Wed, 25 Oct 2023 22:14:49 +0000
Message-ID: <SJ0PR84MB20855ADDEE46D6A437D8E7C18DDEA@SJ0PR84MB2085.NAMPRD84.PROD.OUTLOOK.COM>
References: <20230907210601.25284-1-richard.yu@hpe.com>
 <20230907210601.25284-3-richard.yu@hpe.com>
 <2023100212-hyperlink-prolonged-3e18@gregkh>
In-Reply-To: <2023100212-hyperlink-prolonged-3e18@gregkh>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?rPrzuhrclG0WC82JP03wPQ8bWG+RjIERlUYomrCWsYHVczAFChnlZu9QO7Jl?=
 =?us-ascii?Q?O5IqYnMaiqtqn2LA4OkJ30DI2brOX6UHyRCRy21cccosD17vgKc7xGAQMiet?=
 =?us-ascii?Q?HTSiaVqjWTS6aoMG71fPC5UpLXJlbeC/IKxhF2GOCOpxKkx0DmOPYqzW0BOr?=
 =?us-ascii?Q?Ku4DndslJh3ih2i74PuPtJAKSXykfIVvxpKPbX+J1CDkth1GKMOrUgrqCwNc?=
 =?us-ascii?Q?5EsUSqttXFgYvZdcyLIxpTHSC6iuLuJ/tBjkGRQdNfrJnzigoHiS2OACYiAz?=
 =?us-ascii?Q?0iWM/rOSHjKvHUzlqepxmpOFwT3cz+uJisF44q5e5gpfuB8OHOeMB0vKFxaw?=
 =?us-ascii?Q?C2ltJdqmwZBJQElEivu+k/tFuPltljS0/XeH+JbOkWjvL+H0C8/GRj2GE7Fk?=
 =?us-ascii?Q?34IM4EHf/8kvDmd7jUsquBQE4i0JOvY2LzaUZK/I82yCaqV9NXY8jecrC+P9?=
 =?us-ascii?Q?OE4bA3jJe8lkQG0t2hB8+lV1EAxgIkuQzA6xX9ijd9bq/Q1a9sMaKv9FdKu3?=
 =?us-ascii?Q?kLsDKhAEJv5m4QKj3zY65adG1SjQzg7blX6cFgrtfmaVAJ3CRqDWPrzvWW57?=
 =?us-ascii?Q?pF43q4lLg/HaebvZS7qQffxqPjD6c2V8lSaCBzdRL90A2qbh8FPnXprxhHyi?=
 =?us-ascii?Q?DfoK/utEoPtSAbtLca+dXmmUt2ltRt6p1FJ3SAKND4qz0llGg0O0FMxQU43X?=
 =?us-ascii?Q?N7oRCnpXJzQccc6KVB9SI7mG1Yx7K5vUb4CXtJZWxs7SdRxfexmZMexfYG/+?=
 =?us-ascii?Q?/IQItvVUWlfdExSTaXFF4zNgHeQPbW8tbwrVn1MyK3YuCmCi/HqDbceaUM5o?=
 =?us-ascii?Q?DNMPwbAXTiMMnt5i1G6Yz948E0RSMccTpWqn4e0s4fpa6yjSCHGBJX/vPIR2?=
 =?us-ascii?Q?JTP4rByT1Sgf0ktgANB0yH0XrroJryQpiJhR9Hnj0hvolqNBAZk9HW/qvIDt?=
 =?us-ascii?Q?uEJF4RkTzyLYQ7ITfH0Ut67lg8mTsnhmFgu1YtHUqUMpByqJRvekNLgrf33k?=
 =?us-ascii?Q?/xtSb6qxfam0idtQXKyky3iJ4zmyroO8Ddo3McaSy48o+8NxrIpryROZ/XIu?=
 =?us-ascii?Q?ebuxm3uWIGlFD8uGRpL/vcFHnTAKcGYO/Wat8LMM1JukspRJVgOgxBb3jQms?=
 =?us-ascii?Q?agDzTGgb9KD+gZUg9dLY5W5xiGKZgM71Xadcw3xjsy5MXlbsHMGTU8F7B4lo?=
 =?us-ascii?Q?CjBjFF1Bqb9DeIX78CdHBmX+jzWyfctZHXgZHAC7ab+ASTxfBOYvPN6yuDyM?=
 =?us-ascii?Q?jMlLpqDDv/HTLq/q/rdl0Wc2bcd5Q7qKYrBJEh0bhhwBvqfG4nczKpyKMFC3?=
 =?us-ascii?Q?Yyz1IO7CVe9RU9a/CNn974G3MKZn64YfpW5MMN9sFWq7XIZ2Evh11sIBAN8a?=
 =?us-ascii?Q?6Z8Vq1aaJr7DuoyKNAxfpzlQJJqCb9FnF4DzYpAOq1GvCwP+GOOvl1M0HCKK?=
 =?us-ascii?Q?6iL8X1NsfEEEpIjoH1Vi+wF7tofASo8OiR/At9C4LWhdZyMm3wIOtaFFbu0N?=
 =?us-ascii?Q?+5G+6HdRWe3HmgqHamtrDh33G+rAz+OblkEk7FriObT8WJ0dkctCSfZZ0Mow?=
 =?us-ascii?Q?H2rx9d2LfNfwRFNcvAc=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR84MB2085.NAMPRD84.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-Network-Message-Id: d6236060-d9b4-4956-959a-08dbd5a7ce23
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Oct 2023 22:14:49.7506
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: InWeq3+GO0i59xsAy2n+hlL/1rMJyYbHVAIoimbxt7VwzKbwWGDV+n1EBZqCvbCfOwYxENVrU5M88kizU3xEFw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR84MB1525
X-OriginatorOrg: hpe.com
X-Proofpoint-ORIG-GUID: ZT_v14FLbCuOAQdemG7tZdkW3fwX0CjC
X-Proofpoint-GUID: ZT_v14FLbCuOAQdemG7tZdkW3fwX0CjC
X-HPE-SCL: -1
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-25_12,2023-10-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 suspectscore=0
 spamscore=0 adultscore=0 lowpriorityscore=0 bulkscore=0 priorityscore=1501
 malwarescore=0 mlxlogscore=558 phishscore=0 clxscore=1011 impostorscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001
 definitions=main-2310250191
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 25 Oct 2023 15:16:41 -0700 (PDT)

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

I am using aspeed-vhub as example to write this gxp-hub driver. My struct g=
xp_udc_drvdata{}
Is similar to the struct ast_vhub_dev{} in drivers/usb/gadget/udc/aspeed-vh=
ub/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 i=
n the obmc-ikvm.
The modification is just changing the dev_name to be "80400800.usb-hub". I =
have made sure=20
that the KVM is working. (The virtual keyboard and virtual mouse are workin=
g).

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

./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
1
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
2
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
3
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
4


>> +	/*
>> +	 * 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 =3D kzalloc(sizeof(*drvdata->port_dev), GFP_KERNEL);
>> +	if (!drvdata->port_dev) {
>> +		rc =3D -ENOMEM;
>> +		goto fail_alloc;
>> +	}
>> +	device_initialize(drvdata->port_dev);
>> +	drvdata->port_dev->release =3D gxp_udc_dev_release;
>> +	drvdata->port_dev->parent =3D parent;
>> +	dev_set_name(drvdata->port_dev, "%s:p%d", dev_name(parent), idx +=20
>> +1);
>> +
>> +	/* DMA setting */
>> +	drvdata->port_dev->dma_mask =3D parent->dma_mask;
>> +	drvdata->port_dev->coherent_dma_mask =3D parent->coherent_dma_mask;
>> +	drvdata->port_dev->bus_dma_limit =3D parent->bus_dma_limit;
>> +	drvdata->port_dev->dma_range_map =3D parent->dma_range_map;
>> +	drvdata->port_dev->dma_parms =3D parent->dma_parms;
>> +	drvdata->port_dev->dma_pools =3D parent->dma_pools;
>> +
>> +	rc =3D device_add(drvdata->port_dev);

> So you createad a "raw" device that does not belong to any bus or type an=
d add it to sysfs?=20
>  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:p=
1
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
2
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
3
./sys/bus/platform/devices/ahb@80000000/80400800.usb-hub/80400800.usb-hub:p=
4

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

>> +	if (rc)
>> +		goto fail_add;
>> +
>> +	/* Populate gadget */
>> +	gxp_udc_init(drvdata);
>> +
>> +	rc =3D usb_add_gadget_udc(drvdata->port_dev, &drvdata->gadget);
>> +	if (rc !=3D 0) {
>> +		dev_err(drvdata->port_dev, "add gadget failed\n");
>> +		goto fail_udc;
>> +	}
>> +	rc =3D 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 fol=
lowed the aspeed-vhub driver. The=20
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=20
Available flag and none are working for me, except "IRQF_SHARED". I also co=
nfirmed that the Aspeed-vhub=20
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 th=
ere is potential double free in
the previous review. I ran through the error exit test cases and did not se=
e 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.