Re: WARNING in usb_composite_setup_continue

[Peter Chen <peter.chen@nxp.com>]

On 20-11-13 12:00:15, Alan Stern wrote:
> On Fri, Nov 13, 2020 at 10:02:58AM +0000, Peter Chen wrote:
> > On 20-11-12 10:59:05, Alan Stern wrote:
> > > This is why I think we will need to change the API.  There has to be a 
> > > way to tell usb_composite_setup_continue() which SETUP it is being 
> > > called for.  A new SETUP or a disconnect should invalidate the old 
> > > SETUP, and then usb_composite_setup_continue() would ignore any calls 
> > > that were for the old SETUP packet.
> > > 
> > 
> > I think usb_composite_setup_continue logic has no issue, we only need to
> > consider if changing WARN to DBG is necessary, FuzzUSB seems to be
> > trigger panic for WARN.
> 
> Yes, changing the WARN to DBG will make FuzzUSB happy.  But I still 
> think there is a logical flaw in the design of the API.
> 
> > See below message on its trace log
> > > Kernel panic - not syncing: panic_on_warn set ...
> > 
> > 
> > For new SETUP, current composite layer makes sure the pending request
> > will not get the status since the stage request is only queued when
> > cdev->delayed_status is 1.
> > 
> > But the UDC driver should make sure no new
> > request if old request has not finished, consider the corner case that
> > the new SETUP comes after the pending request calls usb_composite_setup_continue
> > and queues the status stage, then, the two zero-length packets from device
> > will be the bus, but host only wants get one. Meanwhile, there is only one
> > request for composite device (see: usb_composite_dev.req), it means the
> > composite layer can't handle multiple ep0 request.
> 
> That's right.  Unfortunately, I think fixing this will require changes 
> to the UDC drivers as well as to the composite core.  Similar to what I 
> said earlier, there will have to be a way for the composite core to tell 
> the UDC driver which SETUP packet the zero-length status reply is meant 
> for.

It needs the composite layer to support multiple requests for EP0, the
effort is big. It is better the real problem is reported, then, it has
environment to test the solution. The reported FuzzUSB issue is not this issue.

> 
> > For disconnect event, it is an unexpected event between SETUP(DATA) stage
> > and status stage, and usb_composite_setup_continue just does nothing
> > since the bus has already not at configured state.
> 
> Yes.  But there still is another problem in the API.
> 
> Suppose the host sends a Set-Interface request, and the function driver 
> is not able to handle it (maybe a memory allocation fails).  The gadget 
> should report this failure to the host by STALLing ep0.  But there is no 
> way for the function driver to tell the composite core that the failure 
> occurred!
> 
> You can see this problem in f_mass_storage.c.  If do_set_interface() 
> encounters an error, it will return a negative error code.  But the 
> caller (handle_exception()) ignores the return code!
> 
> When Dave Brownell designed the Gadget and Composite APIs, he really did 
> not give sufficient thought to the issues involved in delayed responses 
> to control-OUT transfers.
> 

We could add one parameter for usb_composite_setup_continue to indicate
error occurred during function's deferred operation, and stall the ep0
at usb_composite_setup_continue, do you think so?

-- 

Thanks,
Peter Chen