Re: [PATCH 1/2] usb: chipidea: udc: limit usb request length to max 16KB

Peter Chen
2024-09-14T02:04:22Z

On 24-09-13 23:25:13, Xu Yang wrote:
> On Fri, Sep 13, 2024 at 05:53:44PM +0800, Peter Chen wrote:
> > On 24-09-13 15:11:33, Xu Yang wrote:
> > > On Fri, Sep 13, 2024 at 09:20:45AM +0800, Peter Chen wrote:
> > > > On 24-09-12 12:51:49, Xu Yang wrote:
> > > > > Currently, the deivice controller has below limitations:
> > > > > 1. can't generate short packet interrupt if IOC not set in dTD. So if one
> > > > > request span more than one dTDs and only the last dTD set IOC, the usb
> > > > > request will pending there if no more data comes.
> > > > > 2. the controller can't accurately deliver data to differtent usb requests
> > > > > in some cases due to short packet. For example: one usb request span 3
> > > > > dTDs, then if the controller received a short packet the next packet
> > > > > will go to 2nd dTD of current request rather than the first dTD of next
> > > > > request.
> > > > >
> > > >
> > > > Are there any IP errata for it?
> > >
> > > No. It's decided by hw IP design. This old design may not suit current
> > > requirements.
> > >
> > > >
> > > > > To let the device controller work properly, one usb request should only
> > > > > correspond to one dTD. Then every dTD will set IOC. In theory, each dTD
> > > > > support up to 20KB data transfer if the offset is 0. Due to we cannot
> > > > > predetermine the offset, this will limit the usb request length to max
> > > > > 16KB. This should be fine since most of the user transfer data based on
> > > > > this size policy.
> > > > >
> > > > > Although these limitations found on OUT eps, we can put the request to IN
> > > > > eps too, this will benefit the following patches.
> > > >
> > > > Since IN endpoints have not found the problem, please limit the changes
> > > > only for OUT endpoints.
> > >
> > > This 1st patch is mainly used to serve the 2nd patch which may impact
> > > both IN and OUT eps.
> > ...
> > > Because it's hard to judge whether a request is
> > > suit for transfer if it spans more dTDs. So it's needed for both eps.
> >
> > Sorry, I do not understand you above words. First, you may know this
> > request is for IN or OUT, second, according to TD size and data buffer
> > address, you may know you use one or more dTDs.
>
> If req.num_sgs = 0, then we can know how many TDs need to transfer data.
>
> For example:
> req.buf = 0xA0001800 req.length = 40KB
>
> - TD1 addr:0xA0001800 size:18KB
> - TD2 addr:0xA0017000 size:20KB
> - TD3 addr:0xA002D000 size:2KB
>
> We basically won't meet issue for non-sg case. The only expection is that
> received short packet on TD1 (or TD2). Then the next data packet will go
> to TD2. But it should go to TD1 of next request.
>
> But if num_sgs > 0, we need to check validity of each sg entry due to above
> limitations.
>
> For example:
> req.num_sgs = 3 req.length = 40KB
>
> - sg1.addr = 0xA0001800 length = 18KB -> TD1
> - sg2.addr = 0xA0016000 length = 20KB -> TD2
> - sg3.addr = 0xA0028800 length = 2KB -> TD3
>
> This request can be safty used to transfer data. But we can also meet
> previous short packet issue.
>
> req.num_sgs = 5 req.length = 10B + 20KB
>
> - sg1.addr = 0xA0001800 length = 10B -> TD1
> - sg2.addr = 0xA0016000 length = 6KB -> TD2
> - sg3.addr = 0xA0028800 length = 6KB -> TD3
> - sg4.addr = 0xA003A000 length = 4KB -> TD3
> - sg5.addr = 0xA004C000 length = 4KB -> TD3
>

With your the 2nd patch, you could make end of sg1.addr is PAGE aligned,
in that case, the sg1 and sg2 could be at the one TD. sg1 is at the
first dTD, and sg2 at the 2nd & 3rd dTD. If that could be done, the
host may not see short packet, anyway, you could confirm through
analyser.

Peter

> This request can't be used to transfer data since sg1 + sg2 can't
> form a data packet. The host will see a short packet (100 bytes).
>
> req.num_sgs = 5 req.length = 20KB + 10B
>
> - sg1.addr = 0xA0001800 length = 2KB -> TD1
> - sg2.addr = 0xA0016400 length = 5KB -> TD2
> - sg3.addr = 0xA0028800 length = 8KB -> TD3
> - sg4.addr = 0xA003A800 length = 5KB -> TD4
> - sg5.addr = 0xA004C200 length = 10B -> TD5
>
> Compared to previous request, it need 5 TDs even though req.length
> are same. Most of the sg entries can't share same TD since their
> address is not page aligned. For high-speed isoc eps, sg1 + sg2 can't
> form a 3KB DATA2 + DATA1 + DATA0 data sequence too.
>
> Therefore, it's a bit complicated to validate request if num_sgs > 0,
> especially when req.length is larger than 16KB (1 TD size).
>
> When add such condition, each of the sg entry must follow below
> requirements:
> 1. the end address of the first sg buffer must be 4KB aligned.
> 2. the start and end address of the middle sg buffer must be 4KB aligned.
> 3. the start address of the last sg buffer must be 4KB aligned.
>
> So it will be more easy to validate the request.
>
> Hope this will help you understand the motivation of 1st patch.
>
> Thanks,
> Xu Yang