Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Felipe Balbi <balbi@kernel.org>
To:     Alan Stern <stern@rowland.harvard.edu>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     Paul Elder <paul.elder@ideasonboard.com>, Bin Liu <b-liu@ti.com>,
        kieran.bingham@ideasonboard.com, gregkh@linuxfoundation.org,
        USB list <linux-usb@vger.kernel.org>,
        Kernel development list <linux-kernel@vger.kernel.org>,
        rogerq@ti.com
Subject: Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage
In-Reply-To: <Pine.LNX.4.44L0.1811021437410.1469-100000@iolanthe.rowland.org>
References: <Pine.LNX.4.44L0.1811021437410.1469-100000@iolanthe.rowland.org>
Date:   Tue, 06 Nov 2018 13:24:56 +0200
Message-ID: <87a7mmv46v.fsf@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> There's a similar race at the hardware level.  What happens if the
> controller receives a new SETUP packet and concurrently the driver is
> setting up the controller registers for a response to an earlier
> SETUP?  I don't know how real controllers handle this.

That's HW implementation detail. DWC3, for instance, will ignore the
TRBs and return me the status "setup packet pending". Then I just start
a new SETUP TRB.

>> > > I wonder if there's really a use case for delaying the data stage of
>> > > control OUT requests, as it seems to me that we can perform the
>> > > asynchronous validation of the setup and data stages together, in wh=
ich
>> > > case we would always proceed to the data stage, and only potentially
>> > > delay the status stage. However, if we switch to an explicit API whe=
re
>> > > the transition from the setup to the data stage is triggered by queu=
eing
>> > > a request, and given that such a transition may need to be delayed f=
or
>> > > the control IN case, delaying the data stage for control OUT would
>> > > essentially come for free.
>>=20
>> What do you think about this ? Should we allow function drivers to delay=
 the=20
>> data stage of control OUT requests ?
>
> You mean, should we allow function drivers to queue the data-stage
> request after the setup handler has returned?  I don't see any reason

that's already done:

static void dwc3_ep0_xfer_complete(struct dwc3 *dwc,
			const struct dwc3_event_depevt *event)
{
	struct dwc3_ep		*dep =3D dwc->eps[event->endpoint_number];

	dep->flags &=3D ~DWC3_EP_TRANSFER_STARTED;
	dep->resource_index =3D 0;
	dwc->setup_packet_pending =3D false;

	switch (dwc->ep0state) {
	case EP0_SETUP_PHASE:
		dwc3_ep0_inspect_setup(dwc, event);
		break;
[...]
}

static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
		const struct dwc3_event_depevt *event)
{
	struct usb_ctrlrequest *ctrl =3D (void *) dwc->ep0_trb;
	int ret =3D -EINVAL;
	u32 len;

	if (!dwc->gadget_driver)
		goto out;

	trace_dwc3_ctrl_req(ctrl);

	len =3D le16_to_cpu(ctrl->wLength);
	if (!len) {
		dwc->three_stage_setup =3D false;
		dwc->ep0_expect_in =3D false;
		dwc->ep0_next_event =3D DWC3_EP0_NRDY_STATUS;
	} else {
		dwc->three_stage_setup =3D true;
		dwc->ep0_expect_in =3D !!(ctrl->bRequestType & USB_DIR_IN);
		dwc->ep0_next_event =3D DWC3_EP0_NRDY_DATA;
	}
[...]
}

static int __dwc3_gadget_ep0_queue(struct dwc3_ep *dep,
		struct dwc3_request *req)
{
	struct dwc3		*dwc =3D dep->dwc;

	req->request.actual	=3D 0;
	req->request.status	=3D -EINPROGRESS;
	req->epnum		=3D dep->number;

	list_add_tail(&req->list, &dep->pending_list);

[...]

	if (dwc->three_stage_setup) {
		unsigned        direction;

		direction =3D dwc->ep0_expect_in;
		dwc->ep0state =3D EP0_DATA_PHASE;

		__dwc3_ep0_do_control_data(dwc, dwc->eps[direction], req);

		dep->flags &=3D ~DWC3_EP0_DIR_IN;
	}

	return 0;
}

Regardless of the direction, control data always depends on a call to
usb_ep_queue()

> why not.  After all, some drivers may require this.  Likewise for the=20
> data stage of a control-IN.
>
> Another thing we should do is give function drivers a way to send a
> STALL response for the status stage.  Currently there's no way to do
> it, if a data stage is present.

Status stage can only be stalled if host tries to move data on the wrong
direction. Currently, that's handled internally by UDCs since that's
easy enough to track.

Data stage already has explicit stall handling.

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlvheggACgkQzL64meEa
mQYUWQ//QKqUTzvDjFhnwAxDZtYUGagYIOnk98FI3Xrn7rHyA3jGMfnqbe55mVO7
FvaNwtYZTI0ADk8o63EkfW2Ey8HNtn7ejyQROHMKM8jrPdf9O+00fryqnhbXkKJk
ZdCys12+4MMWCfDq2XR1bvH98Xy949m8XiVNFN3IYkZ4a9GXyQ0B8zQ/gqP7nP36
bO3OyJOoj2ffXLITRyT37j3v6MqsFS80B+ioQKp0I41rCJnXBRsisa2HSsM3iDtN
z2GF7keXPfORIjblqvM13Dunxhfze11Tz+BwQfcmghzkur3KIS3zRpsur9tL8fI7
Y8XwzKeMjSsIrqOOT019Cqj3hhNzt797qGbHT5kfNlK8ZTsm8LeLK9WPd6GQ2gzu
oyvha7EMFo5RPzcOER++e3WxgXrLOSBL5AvPsuUXUWLdE2Ao79QLqE0r8UfoXtfn
tcCWLzxPtzSvxWfF8/6JNiNHiXu76pn6w0ZTDHLPEtkVJew9khmiyWsbSqehteTO
hcMtY1ipIlbjwJZm/33NY3iYlSzkFKLOrWLcFcRy/wcpNRm3wquJ5/aXy4JH3EYc
3ustJuOzM3CcH3uvJX3h+WkK8VBR/EcMsJ266e+KW0eS5cJbmPJFJETinJnNfHY4
D5KPU/5XdS9H19nqrw4PY/QjwgtkY1bkrKklZFzkLUbZXcqK5Kc=
=sMbS
-----END PGP SIGNATURE-----
--=-=-=--