Re: [RFC/PATCH 2/3] usb:gadget: Add SuperSpeed function power management

["Amit Blay" <ablay-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

Hi Felipe,

>> Your question is regarding why we added the function->func_suspend &
>> function->get_status callbacks in the first place? (I'm asking because
>> this is not covered by this patch...)
>
> yes, that's really unnecessary.
>
>> If we let both requests to be handled by function->setup(), then to
>> support SuperSpeed, we'll have to change each and every function's
>> setup()
>> to respond back with a correct default value.
>> The advantage of adding function->func_suspend & function->get_status is
>> that if the function doesn't supply those functions, the default can be
>> handled by the composite setup() function.
>
> just handle as any other USB request. When it's implemented by the
> gadget driver, we will handle it and return success, otherwise (namely
> if f->setup() can't handle it) we stall.

Then, you will not be able to support SuperSpeed without changing each
function to handle those requests in f->setup(). Some hosts may request to
get a status of an interface during enumeration, and the device will
stall.
I understand your point, but I still think that it makes sense to have
both callbacks, and have a default handling in composite.c. The
composite.c is a right place for implementing common ch9 logic, which you
will not want to re-implement for each function, and I think that this is
the case.

>> Yes you are right, the function should handle this. The next patch
>> (usb:gadget: SuperSpeed function power management testing support) adds
>> this exact capability to f_sourcesink. The function invokes the UDC's
>> func_wake callback.
>
> not sure this is the right thing to do though.

Can you please elaborate, what is not the right thing? ;-)
Are you referring to your comments for the other patch?

>> But in this change above, I tried (with minimal changes) to keep the
>> current functionality of gadget zero's autoresume, which uses
>> usb_gadget_wakeup(). Since there is no device remote wakeup in
>> SuperSpeed,
>> only function wake, calling usb_gadget_wakeup() has no real meaning in
>> non-SuperSpeed speeds.
>>
>> The complete solution will be to move the autoresume feature from gadget
>> zero into the functions, f_sourcesink & f_loopback. This is the way
>> wakeup
>
> you shouldn't simply move it there. USB2 still has remote wakeup right ?

Yes, USB2 remote wakeup will work, it will be triggered by a function
instead of by the gadget, but it will still send a USB2 device remote
wakeup to the host. (this is only for gadget zero, which is the only
gadget that seems to be using usb_gadget_wakeup for remote wakeup).

>> should be done in SuperSpeed, as I understand it. That means that both
>> functions will arm a timer on device suspend. When the timer expires, in
>> SuperSpeed it should call the UDC's func_wake callback. For lower
>> speeds,
>> it should call usb_gadget_wakeup to trigger device remote wakeup.
>> What do you think?
>
> not sure. To me, you should only to a remote wakeup (or function wake)
> if the user wants to use the device. Otherwise, why are you waking up
> the host ?

No, no. This is the same mechanism that is *already* implemented by the
zero gadget alone ("autoresume"). I just described a way we can keep this
working for USB3 (SuperSpeed and lower Speeds). The same logic could be
implemented in the two functions, and not by the zero gadget, which really
makes sense for SuperSpeed, and I think that in g_zero case, it makes
sense for USB2 as well (see my previous comment).

>> This change is meant to keep usb_gadget_wakeup() API backwards
>> compatible,
>> meaning that this API will still work in SuperSpeed.
>> Of course, if a new USB class will utilize function wake, the new
>> function
>> will call gadget->ops->func_wakeup().
>
> then change it correctly, don't just hack around it ;-)
>
>> The solution I suggested above will address this comment as well, but
>> the
>> downside is that it is not backward compatible, meaning, it requires to
>> change each gadget that is using usb_gadget_wakeup().
>
> so ? It won't break anything if you do it right. Only USB3-capable
> gadget drivers have function wakeup... so start with the functions which
> have USB3 descriptors...

I agree. The first gadget is g_zero. So please see my previous comments
discussing how g_zero and it's functions can be modified to keep it's
remote wakeup working for USB3.

Thanks Felipe for your comments,
Amit.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html