On Wed, Jun 23, 2021 at 02:44:31PM -0700, Wesley Cheng wrote:
>
>
> On 6/23/2021 4:35 AM, Greg KH wrote:
> > On Wed, Jun 23, 2021 at 02:38:55AM -0700, Wesley Cheng wrote:
> >>
> >>
> >> On 6/21/2021 11:05 PM, Greg KH wrote:
> >>> On Mon, Jun 21, 2021 at 10:27:09PM -0700, Wesley Cheng wrote:
> >>>>
> >>>>
> >>>> 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.
> >>>
> >>> Ah, it's a dereference of an array element. Subtle. Tricky. Messy :(
> >>>
> >>>>>> + 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.
> >>
> >> Hi Greg,
> >>
> >>>
> >>> We have macros to tell if an endpoint is IN or OUT, please use those.
> >>>
> >>> And this "cram the whole thing into 64 bits" is not obvious at all.
> >>> Just pass around the original pointer to the descriptors if someone
> >>> wants to use it or not, don't make up yet-another-data-structure here
> >>> for no good reason. We aren't so memory constrained we need to pack
> >>> stuff into bits here.
> >>>
> >>
> >> Hmm ok, what I can do is to move this logic into the check_config()
> >> callback itself, which is implemented by the UDC driver. So now, the
> >> DWC3 will have to do something similar to what is done here, ie loop the
> >> EP descriptors for each function to determine the number of IN endpoints
> >> being used.
>
> Hi Greg,
>
> >
> > We have common USB core functions for this, why can't you just use them?
> >
>
> So, I've tried to use pre-existing mechanisms there in the USB core, but
> they are not populated correctly at the time of function binding. I
> will highlight some of the things I've tried, and why they do not work.
> If possible, if you could point which core functions can achieve what
> we are trying to do here, that would help as well.
We have functions to detect the IN/OUT of an endpoint, use them.
We also have functions that determine how many endpoints of each type
that a device has, why can you not use them as well? Are they only
valid for driver structures, not gadget ones?
> - f->endpoints - This is a bitmap which carries the endpoints used by
> a particular function driver. This does not work, as this is set during
> receiving the SET_CONFIG packet. (we need this during the function
> driver binding stage)
>
> - gadget->in_epnum/gadget->out_epnum - This carries the count of
> endpoints used per configuration. This would be perfect, but this count
> is only incremented when we are not matching EPs using the EP name. So
> in designs where the EP name is used to match, it can not be used.
>
> - gadget->ep_list - I can use this now in the check_config() to iterate
> through the list of eps to see which ones have been claimed for a
> particular configuration.
>
> So just to re-iterate, the TXFIFO resize logic kicks in when the host
> sends the SET_CONFIG packet, which is the "end" of USB enumeration. We
> had discussed a concern previously where, what if we run the resize
> logic, and there is not enough internal memory. We'd end up with an
> enumerated device w/ certain functions broken.
Where are you running out of memory? In the gadget kernel? Or
somewhere else?
> This is where the check_config() comes into the picture. It uses the
> number of endpoints collected during the bind() stage, and checks to
> make sure the resize logic can at least allocate 1 TXFIFO per endpoint.
> If it can not, then it will fail the bind sequence.
That's fine, I am just worried about your crazy "pack all the bits into
a u64 for no good reason" logic here.
thanks,
greg k-h
