Received-SPF: pass (google.com: domain of linux-kernel+bounces-40657-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ff0bff8b-f26a-87bd-9762-9f2af98abcca@quicinc.com>
Date: Fri, 26 Jan 2024 13:13:38 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v12 04/41] usb: host: xhci-mem: Cleanup pending secondary
 event ring events
Content-Language: en-US
From: Wesley Cheng <quic_wcheng@quicinc.com>
To: Mathias Nyman <mathias.nyman@linux.intel.com>,
        <srinivas.kandagatla@linaro.org>, <mathias.nyman@intel.com>,
        <perex@perex.cz>, <conor+dt@kernel.org>, <corbet@lwn.net>,
        <gregkh@linuxfoundation.org>, <lgirdwood@gmail.com>,
        <andersson@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <konrad.dybcio@linaro.org>, <Thinh.Nguyen@synopsys.com>,
        <broonie@kernel.org>, <bgoswami@quicinc.com>, <tiwai@suse.com>,
        <robh+dt@kernel.org>, <agross@kernel.org>
CC: <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-sound@vger.kernel.org>, <linux-usb@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <alsa-devel@alsa-project.org>
References: <20240102214549.22498-1-quic_wcheng@quicinc.com>
 <20240102214549.22498-5-quic_wcheng@quicinc.com>
 <734591a1-50b4-6dc7-0b93-077355ec12e4@linux.intel.com>
 <7b2ec96b-b72f-c848-7c35-36e61a4072ac@quicinc.com>
 <b254f73b-a1bc-3dd4-f485-a3acf556835d@quicinc.com>
 <2178e799-2068-7443-59b2-310dfdd1ddee@linux.intel.com>
 <ae64ce69-dc1b-1534-7950-0a35c4a56f58@quicinc.com>
In-Reply-To: <ae64ce69-dc1b-1534-7950-0a35c4a56f58@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

Hi Mathias,

On 1/16/2024 12:24 PM, Wesley Cheng wrote:
> Hi Mathias,
> 
> On 1/15/2024 6:01 AM, Mathias Nyman wrote:
>> On 10.1.2024 1.42, Wesley Cheng wrote:
>>> Hi Mathias,
>>>
>>> On 1/8/2024 12:51 PM, Wesley Cheng wrote:
>>>> Hi Mathias,
>>>>
>>>> On 1/4/2024 6:48 AM, Mathias Nyman wrote:
>>>>> On 2.1.2024 23.45, Wesley Cheng wrote:
>>>>>> As part of xHCI bus suspend, the XHCI is halted.  However, if 
>>>>>> there are
>>>>>> pending events in the secondary event ring, it is observed that 
>>>>>> the xHCI
>>>>>> controller stops responding to further commands upon host or device
>>>>>> initiated bus resume.  Iterate through all pending events and 
>>>>>> update the
>>>>>> dequeue pointer to the beginning of the event ring.
>>>>>>
>>>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>>> ...
>>>>>> +/*
>>>>>> + * Move the event ring dequeue pointer to skip events kept in the 
>>>>>> secondary
>>>>>> + * event ring.  This is used to ensure that pending events in the 
>>>>>> ring are
>>>>>> + * acknowledged, so the XHCI HCD can properly enter 
>>>>>> suspend/resume. The
>>>>>> + * secondary ring is typically maintained by an external component.
>>>>>> + */
>>>>>> +void xhci_skip_sec_intr_events(struct xhci_hcd *xhci,
>>>>>> +    struct xhci_ring *ring,    struct xhci_interrupter *ir)
>>>>>> +{
>>>>>> +    union xhci_trb *erdp_trb, *current_trb;
>>>>>> +    u64 erdp_reg;
>>>>>> +    u32 iman_reg;
>>>>>> +    dma_addr_t deq;
>>>>>> +
>>>>>> +    /* disable irq, ack pending interrupt and ack all pending 
>>>>>> events */
>>>>>> +    xhci_disable_interrupter(ir);
>>>>>> +    iman_reg = readl_relaxed(&ir->ir_set->irq_pending);
>>>>>> +    if (iman_reg & IMAN_IP)
>>>>>> +        writel_relaxed(iman_reg, &ir->ir_set->irq_pending);
>>>>>> +
>>>>>> +    /* last acked event trb is in erdp reg  */
>>>>>> +    erdp_reg = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
>>>>>> +    deq = (dma_addr_t)(erdp_reg & ERST_PTR_MASK);
>>>>>> +    if (!deq) {
>>>>>> +        xhci_err(xhci, "event ring handling not required\n");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    erdp_trb = current_trb = ir->event_ring->dequeue;
>>>>>> +    /* read cycle state of the last acked trb to find out CCS */
>>>>>> +    ring->cycle_state = le32_to_cpu(current_trb->event_cmd.flags) 
>>>>>> & TRB_CYCLE;
>>>>>> +
>>>>>> +    while (1) {
>>>>>> +        inc_deq(xhci, ir->event_ring);
>>>>>> +        erdp_trb = ir->event_ring->dequeue;
>>>>>> +        /* cycle state transition */
>>>>>> +        if ((le32_to_cpu(erdp_trb->event_cmd.flags) & TRB_CYCLE) !=
>>>>>> +            ring->cycle_state)
>>>>>> +            break;
>>>>>> +    }
>>>>>> +
>>>>>> +    xhci_update_erst_dequeue(xhci, ir, current_trb, true);
>>>>>> +}
>>>>>
>>>>> Code above is very similar to the existing event ring processing 
>>>>> parts of xhci_irq()
>>>>> and xhci_handle_event()
>>>>>
>>>>> I'll see if I can refactor the existing event ring processing, 
>>>>> decouple it from
>>>>> event handling so that it could be used by primary and secondary 
>>>>> interrupters with
>>>>> handlers, and this case where we just want to clear the event ring.
>>>>>
>>>>
>>>> Thanks, that makes sense.  Will take a look as well.
>>>>
>>>
>>> How about something like the below?  Tested this on my set up and 
>>> everything looks to be working fine.  Had to add another param to 
>>> struct xhci_interrupters to tell the XHCI interrupt handler to say if 
>>> that particular interrupter wants to skip_events (handling).  This 
>>> way, its something that the class driver utilizing the interrupter 
>>> will have to tell XHCI sideband.  It would allow the user to 
>>> determine if they want to use the interrupter to actually handle 
>>> events or not on the proc running Linux.
>>>
>>
>> Yes, I have something similar.
>> I'll share it soon, just need to
>> clean it up a bit fist.
>>
> 
> Sure, no worries.  Will test it when its available.  Thanks!
> 

Was just wondering if you had the time to clean up the changes?  If not, 
maybe you can provide a patch with whatever you have, and I can try my 
best to clean it up to your liking?  Thanks!

Thanks
Wesley Cheng