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

[Xu Yang <xu.yang_2@nxp.com>]

On Sat, Sep 14, 2024 at 10:04:14AM +0800, Peter Chen wrote:
> 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
> > 
> 

Sorry for reply late.

> With your the 2nd patch, you could make end of sg1.addr is PAGE aligned,

The 2nd patch is used to create a liner buffer for rest of the invalid sg
entries, in this case, sg1 is the first invalid entry and sg1 - sg5 will
be bounced together. So it's not what you think, only sg1 is bounced.

> 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

Yeah, I have considered this way. It could not be done. If only bounce sg1,
the usb controller should only transfer 10 bytes to/from sg1.addr. But TD
doesn't have a param to control it. As a result, data may be inconsistent
since usb controller may transfer illegal data.

Thanks,
Xu Yang

> 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