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
> [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@xxxxxxxx>
>>> Tested-by: Ivan Ivanov <ivan.ivanov@xxxxxxxx>
>>> Tested-by: Andrea della Porta <andrea.porta@xxxxxxxx>
>>> ---
>>> 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
