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

[Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>]

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

> 
> 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