Re: [PATCH v3 4/6] drm: Add Generic USB Display driver

[Alan Stern <stern@rowland.harvard.edu>]

On Tue, Jun 02, 2020 at 05:21:50AM +0000, Peter Stuge wrote:
> > The USB protocol forbids a device from sending a STALL response to a
> > SETUP packet.  The only valid response is ACK.  Thus, there is no way
> > to prevent the host from sending its DATA packet for a control-OUT
> > transfer.
> 
> Right; a STALL handshake only after a DATA packet, but a udc could silently
> drop the first DATA packet if instructed to STALL during SETUP processing.
> I don't know how common that is for the udc:s supported by gadget, but some
> MCU:s behave like that.

It happens from time to time, such as when the host sends a SETUP packet 
that the gadget driver doesn't understand.

> > A gadget driver can STALL in response to a control-OUT data packet,
> > but only before it has seen the packet.
> 
> How can it do that for OUT, and IN if possible there too?

In the way described just above: The gadget driver's SETUP handler tells 
the UDC to stall the data packet.

> Is it related to f->setup() returning < 0 ?

Yes; the composite core interprets such a value as meaning to STALL 
endpoint 0.

> The spec also allows NAK, but the gadget code seems to not (need to?)
> explicitly support that. Can you comment on this as well?

If the gadget driver doesn't submit a usb_request then the UDC will 
reply with NAK.

> > Once the driver knows what the data packet contains, the gadget API
> > doesn't provide any way to STALL the status stage.
> 
> Thanks. I think this particular gadget driver doesn't need to decide late.
> 
> Ideally the control transfers can even be avoided.


On Tue, Jun 02, 2020 at 01:46:38PM +0200, Noralf Trønnes wrote:

> > A gadget driver can STALL in response to a control-OUT data packet,
> > but only before it has seen the packet.  Once the driver knows what
> > the data packet contains, the gadget API doesn't provide any way to
> > STALL the status stage.  There has been a proposal to change the API
> > to make this possible, but so far it hasn't gone forward.
> > 
> 
> This confirms what I have seen in the kernel and the reason I added a
> status request so I can know the result of the operation the device has
> performed.

Does this status request use ep0 or some other endpoint?

> I have a problem that I've encountered with this status request.
> In my first version the gadget would usb_ep_queue() the status value
> when the operation was done and as long as this happended within the
> host timeout (5s) everything worked fine.
> 
> Then I hit a 10s timeout in the gadget when performing a display modeset
> operation (wait on missing vblank). The result of this was that the host
> timed out and moved on. The gadget however didn't know that the host
> gave up, so it queued up the status value. The result of this was that
> all further requests from the host would time out.
> Do you know a solution to this?
> My work around is to just poll on the status request, which returns a
> value immediately, until there's a result. The udc driver I use is dwc2.

It's hard to give a precise answer without knowing the details of how 
your driver works.

There are two reasonable approaches you could use.  One is to have a 
vendor-specific control request to get the result of the preceding 
operation.  This works but it has high overhead -- which may not matter 
if it happens infrequently and isn't sensitive to latency.

The other approach is to send the status data over a bulk-IN endpoint.  
It would have to be formatted in such a way that the host could 
recognize it as a status packet and not some other sort of data packet.  
That way, if the host received a stale status value, it could ignore it 
and move on.

You also may want to give some thought to a "resynchronization" 
protocol, for use in situations where the host times out waiting for a 
response from the device while the device is waiting for something else 
(the host, a vblank, or whatever).  This could be a special control 
request, or you could rely on the host doing a complete USB reset.

Alan Stern