Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 0F497C433CA
Subject: Re: [RFC v5 4/6] usb: gadget: configfs: Check USB configuration
 before adding
To:     Peter Chen <peter.chen@nxp.com>
Cc:     "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "bjorn.andersson@linaro.org" <bjorn.andersson@linaro.org>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "agross@kernel.org" <agross@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "jackp@codeaurora.org" <jackp@codeaurora.org>
References: <20200829055846.19034-1-wcheng@codeaurora.org>
 <20200829055846.19034-5-wcheng@codeaurora.org>
 <20200831022825.GA15756@b29397-desktop>
From:   Wesley Cheng <wcheng@codeaurora.org>
Message-ID: <2631cd0b-66d4-aabc-dc41-9ae5e84fa90d@codeaurora.org>
Date:   Mon, 31 Aug 2020 00:34:51 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200831022825.GA15756@b29397-desktop>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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
>>
> 

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