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

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

Hi Minas,

On 10:46 Mon 27 Nov     , Minas Harutyunyan wrote:
> Hi Andrea,
> 
> On 11/27/23 13:04, Andrea della Porta wrote:
> > 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
> According your patch terminology above 'hcint' is a 'hcintraw' and here 
> asserted only channel halted interrupt. So, channel halted without any 
> reason. Not clear how your patch fix this case?
> 
> Couple of questions related to your tests:
> 1. Which DMA mode used here - buffer or descriptor DMA?
> 2. Which type of transfers testing? Or you can add to this print HCCHARx 
> also.
> 3. Which version of core you use (GSNPSID)?
> 
> Thanks,
> Minas
>

The augmented log is here below (hcchar[chnum] and others added):

[330438.623017] --Host Channel Interrupt--, Channel 4
[330438.623029]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[330438.623039]   hcchar[4] = 0x015c9810, chan->ep_type=3
[330438.626183] --Host Channel Interrupt--, Channel 5
[330438.626198]   hcint 0x00000003, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[330438.626208]   hcchar[5] = 0x01d83200, chan->ep_type=2
[330438.627659] --Host Channel Interrupt--, Channel 3
[330438.627674]   hcint 0x00000021, hcintmsk 0x00000426, hcint&hcintmsk 0x00000020
[330438.627686]   hcchar[3] = 0x01d8d200, chan->ep_type=2
[330438.627703] --Host Channel Interrupt--, Channel 3
[330438.627711]   hcint 0x00000002, hcintmsk 0x00000406, hcint&hcintmsk 0x00000002
[330438.627721]   hcchar[3] = 0x01d8d200, chan->ep_type=2
[330438.627740] dwc2 3f980000.usb: dwc2_hc_chhltd_intr_dma: Channel 3 - ChHltd set, but reason is unknown
[330438.627758] dwc2 3f980000.usb: hcint 0x00000002, intsts 0x04600001
[330438.627798] --Host Channel Interrupt--, Channel 0
[330438.627807]   hcint 0x00000010, hcintmsk 0x00000436, hcint&hcintmsk 0x00000010
[330438.627818]   hcchar[0] = 0x81d8d200, chan->ep_type=2
[330438.631017] --Host Channel Interrupt--, Channel 1
[330438.631025]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[330438.631035]   hcchar[1] = 0x015c9810, chan->ep_type=3
[330438.639019] --Host Channel Interrupt--, Channel 7
[330438.639041]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[330438.639053]   hcchar[7] = 0x015c9810, chan->ep_type=3
[330438.647019] --Host Channel Interrupt--, Channel 6
[330438.647040]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002
[330438.647051]   hcchar[6] = 0x015c9810, chan->ep_type=3
[330438.649015] --Host Channel Interrupt--, Channel 4
[330438.649020]   hcint 0x00000012, hcintmsk 0x00000006, hcint&hcintmsk 0x00000002

we're using buffer DMA mode (dma_desc_enable=0), we are atrss testing via ping flooding
through an LTE modem attached to USB which should mostly do bulk dataflow (confirmed
by ep_type==2). From regdump, GSNPSID = 0x4f54280a.

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