Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <3c576edf-89c3-ccf3-a43f-4ce2c1ced18d@zhaoxin.com>
 <Yi7xJy70XZCA8RyN@kroah.com> <CAL411-o_2PSndEVXfa+ciLukSr5u5w8G9T63d2MpSm2Fpn5QTQ@mail.gmail.com>
 <d95ca5f6-221b-1a22-abbe-10621e2fb219@linux.intel.com>
In-Reply-To: <d95ca5f6-221b-1a22-abbe-10621e2fb219@linux.intel.com>
From:   Peter Chen <hzpeterchen@gmail.com>
Date:   Thu, 17 Mar 2022 09:51:01 +0800
Message-ID: <CAL411-pJt2mZFeE_mkGrA68VhkG237FYwCEUY9FTr6ptUQNd_Q@mail.gmail.com>
Subject: Re: [PATCH] USB: Fix xhci ERDP update issue
To:     Mathias Nyman <mathias.nyman@linux.intel.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        "WeitaoWang-oc@zhaoxin.com" <WeitaoWang-oc@zhaoxin.com>,
        mathias.nyman@intel.com, Alan Stern <stern@rowland.harvard.edu>,
        USB list <linux-usb@vger.kernel.org>,
        lkml <linux-kernel@vger.kernel.org>, CobeChen@zhaoxin.com,
        TimGuo@zhaoxin.com, tonywwang@zhaoxin.com, weitaowang@zhaoxin.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Mar 16, 2022 at 8:43 PM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 16.3.2022 13.57, Peter Chen wrote:
> > On Mon, Mar 14, 2022 at 10:34 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >> On Mon, Mar 14, 2022 at 03:25:23PM +0800, WeitaoWang-oc@zhaoxin.com wrote:
> >>> On some situations, software handles TRB events slower than adding TRBs,
> >>> xhci_irq will not exit until all events are handled. If xhci_irq just
> >>> handles 256 TRBs and exit, the temp variable(event_ring_deq) driver records
> >>> in xhci irq is equal to driver current dequeue pointer. It will cause driver
> >>> not update ERDP and software dequeue pointer lost sync with ERDP. On the
> >>> next xhci_irq, the event ring is full but driver will not update ERDP as
> >>> software dequeue pointer is equal to ERDP.
> >
> > At the current driver, the ERDP is updated at most 128 TRBs, how is
> > the above condition
> > triggered?
> >
> > Peter
> >
>
> Before, and during _one_ interrupt handling xHC hardware writes exactly 256 events
> to event ring. ring buffer size is 256 so buffer position 0 and 256 point
> to the same place.
>
> Interrupt handler stores software dequeue in a local variable "event_ring_deq".
> Handler start handling events, it updates software dequeue, but not local variable.
> After 128 events handler updates hardware ERDP.
>
> So at event 128 we got:
> Hardware ERDP = 128
> software dequeue = 128
> event_ring_deq = 0
>
> Handler continue handling events, at event 256 try to update HW ERDP again, but fail due
> to this condition in update_erst_dequeue():
>       if (event_ring_deq != xhci->event_ring->dequeue)
>
> This fails because event_ring_deq is still 0, and software deq is 256,
> pointing to the same place in the event ring.
>
> So at the end of the interrupt handler we have:
> HW ERDP = 128
> software dequeue = 256 (same as 0)
>
> So in this specific case we fail to update ERDP correctly

Cleared, thanks.

Peter