Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Linyu Yuan <quic_linyyuan@quicinc.com>
Cc: "Thinh Nguyen" <Thinh.Nguyen@synopsys.com>,
	정재훈 <jh0801.jung@samsung.com>,
	"'Felipe Balbi'" <balbi@kernel.org>,
	"'Greg Kroah-Hartman'" <gregkh@linuxfoundation.org>,
	"'open list:USB XHCI DRIVER'" <linux-usb@vger.kernel.org>,
	"'open list'" <linux-kernel@vger.kernel.org>,
	"'Seungchull Suh'" <sc.suh@samsung.com>,
	"'Daehwan Jung'" <dh10.jung@samsung.com>
Subject: Re: [PATCH] usb: dwc3: Clear DWC3_EVENT_PENDING when count is 0
Date: Wed, 11 Jan 2023 02:27:34 +0000	[thread overview]
Message-ID: <20230111022720.mcrhdrthgwlwszcv@synopsys.com> (raw)
In-Reply-To: <826538ae-d27a-fc03-c8dc-94b53c8a44cf@quicinc.com>

On Wed, Jan 11, 2023, Linyu Yuan wrote:
> 
> On 1/11/2023 8:00 AM, Thinh Nguyen wrote:
> > 
> > > one more question, is it legacy PCIe device still exist in real world ? and
> > > any VID/PID info ?
> > Currently, all dwc3 PCIe devices are affected. Some setups are more
> 
> 
> if non PCIe device have no such issue, can we do some improvement for it ?
> 
> like new flag or static key/jump label to improve interrupt handler ?
> 

There are different ways to handle this. At the time, it was (and is?)
the simplest solution. There isn't any observable performance
degradation from testing so there's no incentive to change it yet.

BR,
Thinh

> 
> > noticeable than others. The dwc3 driver is implemented to probe platform
> > devices. So, dwc3 PCIe devices are wrapped as platform devices for the
> > dwc3 driver. Since we're going through the platform device code path,
> > the pci layer falls back to using legacy interrupt instead of MSI (last
> > I check awhile ago).
> > 
> > A little more detail on this problem:
> > PCIe legacy interrupt will emulate interrupt line by sending an
> > interrupt assert and deassert messages. After the interrupt assert
> > message is sent, interrupts are continuously generated until the
> > deassert message is sent. If there's a register write to unmask/mask
> > interrupt or clearing events falls in between these messages, then there
> > may be a race.
> > 
> > Let's say we don't have event pending check, this can happen:
> > 
> > Normal scenario
> > ---------------
> >      event_count += n # controller generates new events
> >    interrupt asserts
> >      write(mask irq)
> >      event_count -= n # dwc3 clears events
> >    interrupt deasserts
> >      write(unmask irq)
> > 
> > 
> > Race scenario
> > -------------
> >      event_count += n # new events
> >    interrupt asserts
> >      write(mask irq)
> >      event_count -= n # clear events
> >      event_count += n # more events come and hard irq handler gets called
> > 		     # again as interrupt is generated, but cached
> > 		     # events haven't been handled. This breaks
> > 		     # serialization and causes lost events.
> >      write(mask irq)
> > 
> >      event_count -= n # clear events
> >    interrupt deasserts
> >      write(unmask irq) # events handled
> 
> 
> if mask irq is not working, the race will happen like this, thanks for
> explanation.
> 
> 
> > 
> > For MSI, this won't be a problem because it's edge-triggered and the way
> > it sends interrupt is different.
> > 