Re: [PATCH v10 2/6] usb: gadget: configfs: Check USB configuration before adding

[Wesley Cheng]

On 6/17/2021 4:07 AM, Greg KH wrote:
> On Thu, Jun 17, 2021 at 02:58:15AM -0700, 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@xxxxxxxxxxxxxx>
>> ---
>>  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 15a607c..76b9983 100644
>> --- a/drivers/usb/gadget/configfs.c
>> +++ b/drivers/usb/gadget/configfs.c
>> @@ -1374,6 +1374,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;
>> @@ -1403,7 +1404,28 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
>>  				list_add(&f->list, &cfg->func_list);
>>  				goto err_purge_funcs;
>>  			}
>> +			if (f->fs_descriptors) {
>> +				struct usb_descriptor_header **d;
>> +
>> +				d = f->fs_descriptors;
>> +				for (; *d; ++d) {

Hi Greg,

Thanks for the review and feedback.

> 
> With this check, there really is not a need to check for
> f->fs_descriptors above in the if statement, right?
> 

f->fs_descriptor will carry the table of descriptors that a particular
function driver has assigned to it.  The for loop here, will dereference
the individual descriptors within that descriptor array, so we need to
first ensure the descriptor array is present before traversing through
the individual entries/elements.

>> +					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);
> 
> Don't we have direction macros for this type of check?
> 

I don't believe we have a macro which would be able to convert the
bEndpointAddress field into the bit which needs to be set, assuming that
the 32bit ep_map has the lower 16bits carrying OUT EPs, and the upper
16bits carrying the IN EPs.

>> +					set_bit(addr, &ep_map);
>> +				}
> 
> What is this loop trying to do?  Please document it as I can not figure
> it out at all.
> 

Sure I can add some comments to this loop.  All its trying to do is to
set corresponding EPs being used (both IN and OUT eps) by a particular
USB configuration, then passing this bit mapped value to the DWC3 gadget
to evaluate.  As mentioned in the previous comment, the upper 16 bits
carries all IN eps used, and the lower 16 bits carries all the OUT eps used.

Thanks
Wesley Cheng

> thanks,
> 
> greg k-h
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project