Re: [PATCH] USB: dwc2: write HCINT with INTMASK applied

[Andrea della Porta <aporta@suse.de>]

On 12:35 Wed 22 Nov     , Minas Harutyunyan wrote:
> Hi Oliver,
> 
> On 11/15/23 18:45, Oliver Neukum wrote:
> > dwc2_hc_n_intr() writes back INTMASK as read but evaluates it
> > with intmask applied. In stress testing this causes spurious
> > interrupts like this:
> > 
> > [Mon Aug 14 10:51:07 2023] dwc2 3f980000.usb: dwc2_hc_chhltd_intr_dma: Channel 7 - ChHltd set, but reason is unknown
> > [Mon Aug 14 10:51:07 2023] dwc2 3f980000.usb: hcint 0x00000002, intsts 0x04600001
> > [Mon Aug 14 10:51:08 2023] dwc2 3f980000.usb: dwc2_hc_chhltd_intr_dma: Channel 0 - ChHltd set, but reason is unknown
> > [Mon Aug 14 10:51:08 2023] dwc2 3f980000.usb: hcint 0x00000002, intsts 0x04600001
> > [Mon Aug 14 10:51:08 2023] dwc2 3f980000.usb: dwc2_hc_chhltd_intr_dma: Channel 4 - ChHltd set, but reason is unknown
> > [Mon Aug 14 10:51:08 2023] dwc2 3f980000.usb: hcint 0x00000002, intsts 0x04600001
> > [Mon Aug 14 10:51:08 2023] dwc2 3f980000.usb: dwc2_update_urb_state_abn(): trimming xfer length
> > 
> > Applying INTMASK prevents this. The issue exists in all versions of the
> > driver.
> 
> I'm Ok with this patch, just need some elaboration.
> So, channel halted interrupt asserted due to some other interrupt (AHB 
> Error, Excessive transaction errors, Babble, Stall) which was masked. 
> Can you check which of masked interrupts is cause of channel halt interrupt?
> 
> Thanks,
> Minas
>

Hi Minas,
here's the report from dmesg:

[209384.238724]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.246725]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.247634]   hcint 0x00000003, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.254722]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.262725]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.270724]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.278336]   hcint 0x00000092, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.278384]   hcint 0x00000010, hcintmsk 0x00000436, hcint&hcintmsk 0x00000010
[209384.278720]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.286723]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.288486]   hcint 0x00000021, hcintmsk 0x00000426, hcint&hcintmsk 0x00000020
[209384.288511]   hcint 0x00000002, hcintmsk 0x00000406, hcint&hcintmsk 0x00000002
[209384.288528] dwc2 3f980000.usb: dwc2_hc_chhltd_intr_dma: Channel 1 - ChHltd set, but reason is unknown
[209384.288604]   hcint 0x00000010, hcintmsk 0x00000436, hcint&hcintmsk 0x00000010
[209384.294720]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.302734]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.310724]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.318721]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.321722]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[209384.326729]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002


Many thanks,
Andrea 


> > 
> > Signed-off-by: Oliver Neukum <oneukum@suse.com>
> > Tested-by: Ivan Ivanov <ivan.ivanov@suse.com>
> > Tested-by: Andrea della Porta <andrea.porta@suse.com>
> > ---
> >   drivers/usb/dwc2/hcd_intr.c | 15 +++++++--------
> >   1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> > index 0144ca8350c3..5c7538d498dd 100644
> > --- a/drivers/usb/dwc2/hcd_intr.c
> > +++ b/drivers/usb/dwc2/hcd_intr.c
> > @@ -2015,15 +2015,17 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum)
> >   {
> >   	struct dwc2_qtd *qtd;
> >   	struct dwc2_host_chan *chan;
> > -	u32 hcint, hcintmsk;
> > +	u32 hcint, hcintraw, hcintmsk;
> >   
> >   	chan = hsotg->hc_ptr_array[chnum];
> >   
> > -	hcint = dwc2_readl(hsotg, HCINT(chnum));
> > +	hcintraw = dwc2_readl(hsotg, HCINT(chnum));
> >   	hcintmsk = dwc2_readl(hsotg, HCINTMSK(chnum));
> > +	hcint = hcintraw & hcintmsk;
> > +	dwc2_writel(hsotg, hcint, HCINT(chnum));
> > +
> >   	if (!chan) {
> >   		dev_err(hsotg->dev, "## hc_ptr_array for channel is NULL ##\n");
> > -		dwc2_writel(hsotg, hcint, HCINT(chnum));
> >   		return;
> >   	}
> >   
> > @@ -2032,11 +2034,9 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum)
> >   			 chnum);
> >   		dev_vdbg(hsotg->dev,
> >   			 "  hcint 0x%08x, hcintmsk 0x%08x, hcint&hcintmsk 0x%08x\n",
> > -			 hcint, hcintmsk, hcint & hcintmsk);
> > +			 hcintraw, hcintmsk, hcint);
> >   	}
> >   
> > -	dwc2_writel(hsotg, hcint, HCINT(chnum));
> > -
> >   	/*
> >   	 * If we got an interrupt after someone called
> >   	 * dwc2_hcd_endpoint_disable() we don't want to crash below
> > @@ -2046,8 +2046,7 @@ static void dwc2_hc_n_intr(struct dwc2_hsotg *hsotg, int chnum)
> >   		return;
> >   	}
> >   
> > -	chan->hcint = hcint;
> > -	hcint &= hcintmsk;
> > +	chan->hcint = hcintraw;
> >   
> >   	/*
> >   	 * If the channel was halted due to a dequeue, the qtd list might