Re: [PATCH v2 3/3] usb: chipidea: udc: create bounce buffer for problem sglist entries if possible

Xu Yang
2024-10-09T01:51:25Z

Hi Kees,

On Tue, Oct 08, 2024 at 10:27:15PM +0200, Kees Bakker wrote:
> Op 23-09-2024 om 10:12 schreef Xu Yang:
> > The chipidea controller doesn't fully support sglist, such as it can not
> > transfer data spanned more dTDs to form a bus packet, so it can only work
> > on very limited cases.
> >
> > The limitations as below:
> > 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 first sg buffer must be 4KB aligned.
> >
> > However, not all the use cases violate these limitations. To make the
> > controller compatible with most of the cases, this will try to bounce the
> > problem sglist entries which can be found by sglist_get_invalid_entry().
> > Then a bounced line buffer (the size will roundup to page size) will be
> > allocated to replace the remaining problem sg entries. The data will be
> > copied between problem sg entries and bounce buffer according to the
> > transfer direction. The bounce buffer will be freed when the request
> > completed.
> >
> > Acked-by: Peter Chen <peter.chen@kernel.com>
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> >
> > ---
> > Changes in v2:
> > - judge ci->has_short_pkt_limit
> > - add Ack-by tag
> > ---
> > drivers/usb/chipidea/udc.c | 148 +++++++++++++++++++++++++++++++++++++
> > drivers/usb/chipidea/udc.h | 2 +
> > 2 files changed, 150 insertions(+)
> > [...]
> > @@ -552,6 +673,8 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
> > struct ci_hdrc *ci = hwep->ci;
> > int ret = 0;
> > struct td_node *firstnode, *lastnode;
> > + unsigned int bounced_size;
> > + struct scatterlist *sg;
> > /* don't queue twice */
> > if (hwreq->req.status == -EALREADY)
> > @@ -559,11 +682,29 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, struct ci_hw_req *hwreq)
> > hwreq->req.status = -EALREADY;
> > + if (hwreq->req.num_sgs && hwreq->req.length &&
> > + ci->has_short_pkt_limit) {
> > + ret = sglist_get_invalid_entry(ci->dev->parent, hwep->dir,
> > + &hwreq->req);
> > + if (ret < hwreq->req.num_sgs) {
> bounced_size is only initialized here
> > + ret = sglist_do_bounce(hwreq, ret, hwep->dir == TX,
> > + &bounced_size);
> > + if (ret)
> > + return ret;
> > + }
> > + }
> > +
> > ret = usb_gadget_map_request_by_dev(ci->dev->parent,
> > &hwreq->req, hwep->dir);
> > if (ret)
> > return ret;
> > + if (hwreq->sgt.sgl) {
> > + /* We've mapped a bigger buffer, now recover the actual size */
> > + sg = sg_last(hwreq->req.sg, hwreq->req.num_sgs);
> bounced_size can be uninitialized at this point, if the earlier if condition
> is false.

If sglist_do_bounce() isn't called, hwreq->sgt.sgl is NULL too.

Thanks,
Xu Yang