Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3566 bytes --]


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 which
>> > > case we would always proceed to the data stage, and only potentially
>> > > delay the status stage. However, if we switch to an explicit API where
>> > > the transition from the setup to the data stage is triggered by queueing
>> > > a request, and given that such a transition may need to be delayed for
>> > > the control IN case, delaying the data stage for control OUT would
>> > > essentially come for free.
>> 
>> What do you think about this ? Should we allow function drivers to delay the 
>> 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 = dwc->eps[event->endpoint_number];

	dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
	dep->resource_index = 0;
	dwc->setup_packet_pending = 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 = (void *) dwc->ep0_trb;
	int ret = -EINVAL;
	u32 len;

	if (!dwc->gadget_driver)
		goto out;

	trace_dwc3_ctrl_req(ctrl);

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

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

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

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

[...]

	if (dwc->three_stage_setup) {
		unsigned        direction;

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

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

		dep->flags &= ~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 
> 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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]