Re: [PATCH 1/7] usb: mtu3: give back request when rx error happens

[Chunfeng Yun (云春峰)]

On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 03/04/23 04:52, Chunfeng Yun ha scritto:
> > When the Rx enconnter errors, currently, only print error logs,
> > that
> > may cause class driver's RX halt, shall give back the request with
> > error status meanwhile.
> > 
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> 
>  From what I understand, this is not a new feature, but a fix for a
> unwanted QMU
> halt.
> This means that this commit needs a Fixes tag.
I did not take into account this cases when write this driver, it
caused the issue by the bug of host driver.

> 
> > ---
> >   drivers/usb/mtu3/mtu3_qmu.c | 39
> > ++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/mtu3/mtu3_qmu.c
> > b/drivers/usb/mtu3/mtu3_qmu.c
> > index a2fdab8b63b2..7be4e4be1a6a 100644
> > --- a/drivers/usb/mtu3/mtu3_qmu.c
> > +++ b/drivers/usb/mtu3/mtu3_qmu.c
> > @@ -466,6 +466,39 @@ static void qmu_tx_zlp_error_handler(struct
> > mtu3 *mtu, u8 epnum)
> >       mtu3_qmu_resume(mep);
> >   }
> > 
> > +/*
> > + * when rx error happens (except zlperr), QMU will stop, and RQCPR
> > saves
> > + * the GPD encountered error, Done irq will arise after resuming
> > QMU again.
> > + */
> > +static void qmu_error_rx(struct mtu3 *mtu, u8 epnum)
> > +{
> > +     struct mtu3_ep *mep = mtu->out_eps + epnum;
> > +     struct mtu3_gpd_ring *ring = &mep->gpd_ring;
> > +     struct qmu_gpd *gpd_current = NULL;
> > +     struct usb_request *req = NULL;
> > +     struct mtu3_request *mreq;
> > +     dma_addr_t cur_gpd_dma;
> > +
> > +     cur_gpd_dma = read_rxq_cur_addr(mtu->mac_base, epnum);
> > +     gpd_current = gpd_dma_to_virt(ring, cur_gpd_dma);
> > +
> > +     mreq = next_request(mep);
> > +     if (!mreq || mreq->gpd != gpd_current) {
> > +             dev_err(mtu->dev, "no correct RX req is found\n");
> > +             return;
> > +     }
> > +
> > +     req = &mreq->request;
> > +     req->status = -EAGAIN;
> 
> You don't need a *req pointer for just one simple assignment.
> 
>         mreq->request.status = -EAGAIN;
> 
> that'll do.
That's good, I'll modify it, thanks
> 
> > +
> > +     /* by pass the current GDP */
> > +     gpd_current->dw0_info |= cpu_to_le32(GPD_FLAGS_BPS |
> > GPD_FLAGS_HWO);
> > +     mtu3_qmu_resume(mep);
> > +
> > +     dev_dbg(mtu->dev, "%s EP%d, current=%p, req=%p\n",
> > +             __func__, epnum, gpd_current, mreq);
> > +}
> > +
> >   /*
> >    * NOTE: request list maybe is already empty as following case:
> >    * queue_tx --> qmu_interrupt(clear interrupt pending, schedule
> > tasklet)-->
> > @@ -571,14 +604,18 @@ static void qmu_exception_isr(struct mtu3
> > *mtu, u32 qmu_status)
> > 
> >       if ((qmu_status & RXQ_CSERR_INT) || (qmu_status &
> > RXQ_LENERR_INT)) {
> >               errval = mtu3_readl(mbase, U3D_RQERRIR0);
> > +             mtu3_writel(mbase, U3D_RQERRIR0, errval);
> 
> Please mention in the commit description the reason why you're moving
> this register
> write here.
It clears irq status before handling the error;

> 
> Regards,
> Angelo