Re: [RFC v5 4/6] usb: gadget: configfs: Check USB configuration before adding

Wesley Cheng
2020-08-31T07:35:01Z

On 8/30/2020 7:29 PM, Peter Chen wrote:
> On 20-08-28 22:58:44, Wesley Cheng wrote:
>> Ensure that the USB gadget is able to support the configuration being
>> added based on the number of endpoints required from all interfaces. This
>> is for accounting for any bandwidth or space limitations.
>>
>> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
>> ---
>> drivers/usb/gadget/configfs.c | 22 ++++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
>> index 56051bb97349..7c74c04b1d8c 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1361,6 +1361,7 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
>> struct usb_function *f;
>> struct usb_function *tmp;
>> struct gadget_config_name *cn;
>> + unsigned long ep_map = 0;
>>
>> if (gadget_is_otg(gadget))
>> c->descriptors = otg_desc;
>> @@ -1390,7 +1391,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
>> list_add(&f->list, &cfg->func_list);
>> goto err_purge_funcs;
>> }
>> + if (f->ss_descriptors) {
>> + struct usb_descriptor_header **d;
>> +
>> + d = f->ss_descriptors;
>> + for (; *d; ++d) {
>> + struct usb_endpoint_descriptor *ep;
>> + int addr;
>> +
>> + if ((*d)->bDescriptorType != USB_DT_ENDPOINT)
>> + continue;
>> +
>> + ep = (struct usb_endpoint_descriptor *)*d;
>> + addr = ((ep->bEndpointAddress & 0x80) >> 3) |
>> + (ep->bEndpointAddress & 0x0f);
>
> ">> 3" or "<< 3?
>

Hi Peter,

Thanks for your comments. It should be ">> 3" as we want to utilize the
corresponding USB_DIR_IN bit in the bitmap to set the correct bit.
(USB_DIR_IN = 0x80)

>> + set_bit(addr, &ep_map);
>
> You want to record all endpoints on ep_map? Considering there are
> four EP_IN (1-4), and four EP_OUT (1-4), what the value of ep_map
> would like?
>

So for example, if a configuration uses EP8IN and EP9OUT, then the
ep_map will look like:

EP8-IN:
addr = ((0x88 & 0x80) >> 3) | (0x88 & 0xf) --> 0x18

EP9-OUT:
addr = ((0x9 & 0x80) >> 3) | (0x9 & 0xf) --> 0x9

ep_map = ep_map = 0x01000200

The lower 16 bits will carry the OUT endpoints, whereas the upper 16
bits are the IN endpoints. (ie bit16 = ep0in, bit0 = ep0out)

>> + }
>> + }
>> }
>> + ret = usb_gadget_check_config(cdev->gadget, ep_map);
>> + if (ret)
>> + goto err_purge_funcs;
>> +
>
> You may move this patch after your 4nd patch to avoid "git bisect"
> issue.
>

Sure, thanks for the suggestion, will do that in the next rev.

Thanks
Wesley

>> usb_ep_autoconfig_reset(cdev->gadget);
>> }
>> if (cdev->use_os_string) {
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>