| Message ID | 1505765023.29839.32.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] 8139too: revisit napi_complete_done() usage | expand |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 18 Sep 2017 13:03:43 -0700 > From: Eric Dumazet <edumazet@google.com> > > It seems we have to be more careful in napi_complete_done() > use. This patch is not a revert, as it seems we can > avoid bug that Ville reported by moving the napi_complete_done() > test in the spinlock section. > > Many thanks to Ville for detective work and all tests. > > Fixes: 617f01211baf ("8139too: use napi_complete_done()") > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Applied and queued up for -stable.
On Mon, Sep 18, 2017 at 11:57 PM, David Miller <davem@davemloft.net> wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 18 Sep 2017 13:03:43 -0700 > >> From: Eric Dumazet <edumazet@google.com> >> >> It seems we have to be more careful in napi_complete_done() >> use. This patch is not a revert, as it seems we can >> avoid bug that Ville reported by moving the napi_complete_done() >> test in the spinlock section. >> >> Many thanks to Ville for detective work and all tests. >> >> Fixes: 617f01211baf ("8139too: use napi_complete_done()") >> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Applied and queued up for -stable. Eric, sorry to bring up this old thread, but I had a question. Do we have to surround most usage of napi_complete_done() with a spinlock, or was this problem restricted to just the 8139too driver? - Matthew Whitehead
On Mon, 2018-01-22 at 12:27 -0500, tedheadster wrote: > On Mon, Sep 18, 2017 at 11:57 PM, David Miller <davem@davemloft.net> wrote: > > From: Eric Dumazet <eric.dumazet@gmail.com> > > Date: Mon, 18 Sep 2017 13:03:43 -0700 > > > > > From: Eric Dumazet <edumazet@google.com> > > > > > > It seems we have to be more careful in napi_complete_done() > > > use. This patch is not a revert, as it seems we can > > > avoid bug that Ville reported by moving the napi_complete_done() > > > test in the spinlock section. > > > > > > Many thanks to Ville for detective work and all tests. > > > > > > Fixes: 617f01211baf ("8139too: use napi_complete_done()") > > > Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Applied and queued up for -stable. > > Eric, > sorry to bring up this old thread, but I had a question. Do we have > to surround most usage of napi_complete_done() with a spinlock, or was > this problem restricted to just the 8139too driver? I am not aware of any other NIC driver having a problem there.
diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c index 89631753e79962d91456d93b71929af768917da1..cd2dbec331dd796f5296cd378561b3443f231673 100644 --- a/drivers/net/ethernet/realtek/8139too.c +++ b/drivers/net/ethernet/realtek/8139too.c @@ -2135,11 +2135,12 @@ static int rtl8139_poll(struct napi_struct *napi, int budget) if (likely(RTL_R16(IntrStatus) & RxAckBits)) work_done += rtl8139_rx(dev, tp, budget); - if (work_done < budget && napi_complete_done(napi, work_done)) { + if (work_done < budget) { unsigned long flags; spin_lock_irqsave(&tp->lock, flags); - RTL_W16_F(IntrMask, rtl8139_intr_mask); + if (napi_complete_done(napi, work_done)) + RTL_W16_F(IntrMask, rtl8139_intr_mask); spin_unlock_irqrestore(&tp->lock, flags); } spin_unlock(&tp->rx_lock);