Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Pavel Machek : [...] > Considering the memory barriers... is something like this neccessary > in the via-rhine ? Yes. > AFAICT... we need a barrier after making sure that descriptor is no > longer owned by DMA (to make sure we don't get stale data in rest of > descriptor)... and

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi, On 18.12.2016 19:30, Pavel Machek wrote: > Hi! > >> > - e1efa87241272104d6a12c8b9fcdc4f62634d447 >> >> Yep, a sync of the dma descriptors before the hardware gets ownership of the >> tx tail >> idx is missing in the stmmac, too. > > I can reproduce failure with 4.4 fairly easily. I tried

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi! > Lino, have you considered via-rhine.c since its "move work from irq to > workqueue context" changes that started in > 7ab87ff4c770eed71e3777936299292739fcd0fe [*] ? > > It's a shameless plug - originated in r8169.c - but it should be rather > close to what the sxgbe and friends require.

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi! > > For the same reason it's broken if it races with the transmit path: it > > can release driver resources while the transmit path uses these. > > > > Btw the points below may not matter/hurt much for a proof a concept > > but they would need to be addressed as well: > > 1) unchecked (and

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi! > > - e1efa87241272104d6a12c8b9fcdc4f62634d447 > > Yep, a sync of the dma descriptors before the hardware gets ownership of the > tx tail > idx is missing in the stmmac, too. I can reproduce failure with 4.4 fairly easily. I tried with dma_ variant of barriers, and it failed, too [

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi! > > - e1efa87241272104d6a12c8b9fcdc4f62634d447 > > Yep, a sync of the dma descriptors before the hardware gets ownership of the > tx tail > idx is missing in the stmmac, too. Thanks for the hint. Actually, the driver uses smp_wmb() which is completely crazy, and probably misses rmb() in

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi, On 18.12.2016 01:15, Francois Romieu wrote: > Pavel Machek : > [...] >> Won't this up/down the interface, in a way userspace can observe? > > It won't up/down the interface as it doesn't exactly mimic what the > network code does (there's more than rtnl_lock). > Right.

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Pavel Machek : [...] > Won't this up/down the interface, in a way userspace can observe? It won't up/down the interface as it doesn't exactly mimic what the network code does (there's more than rtnl_lock). For the same reason it's broken if it races with the transmit path: it can

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On Thu 2016-12-15 23:33:22, Lino Sanfilippo wrote: > On 15.12.2016 22:32, Lino Sanfilippo wrote: > > > Ah ok. Then maybe priv->hw->dma->stop_tx() does not do the job correctly > > (stop the > > tx path properly) and the HW is still active on the tx path while the tx > > buffers are > > freed.

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On 15.12.2016 22:32, Lino Sanfilippo wrote: > Ah ok. Then maybe priv->hw->dma->stop_tx() does not do the job correctly > (stop the > tx path properly) and the HW is still active on the tx path while the tx > buffers are > freed. OTOH stmmac_release() also stops the phy before the tx (and rx)

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On 15.12.2016 22:03, Pavel Machek wrote: > > I actually did experiment with adding locking there, too, and no, no > luck. It seems stmmac_tx_err() is more broken than just locking. > Ah ok. Then maybe priv->hw->dma->stop_tx() does not do the job correctly (stop the tx path properly) and the

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi! > sorry for the late reply. No problem. Thanks for the help. > On 11.12.2016 21:11, Pavel Machek wrote: > > > > Do you understand what stmmac_tx_err(priv); is supposed to do? In > > particular, if it is called while the driver is working ok -- should > > the driver survive that? > > As

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi Pavel, sorry for the late reply. On 11.12.2016 21:11, Pavel Machek wrote: > > Do you understand what stmmac_tx_err(priv); is supposed to do? In > particular, if it is called while the driver is working ok -- should > the driver survive that? As far as I understood it is supposed to fixup an

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi! > On 09.12.2016 12:21, Pavel Machek wrote: > > On Fri 2016-12-09 00:19:43, Francois Romieu wrote: > >> Lino Sanfilippo : > >> [...] > >> > OTOH Pavel said that he actually could produce a deadlock. Now I wonder > >> > if > >> > this is caused by that locking scheme

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi, On 09.12.2016 12:21, Pavel Machek wrote: > On Fri 2016-12-09 00:19:43, Francois Romieu wrote: >> Lino Sanfilippo : >> [...] >> > OTOH Pavel said that he actually could produce a deadlock. Now I wonder if >> > this is caused by that locking scheme (in a way I have not

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On Fri 2016-12-09 00:19:43, Francois Romieu wrote: > Lino Sanfilippo : > [...] > > OTOH Pavel said that he actually could produce a deadlock. Now I wonder if > > this is caused by that locking scheme (in a way I have not figured out yet) > > or if it is a different issue. >

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Lino Sanfilippo : [...] > OTOH Pavel said that he actually could produce a deadlock. Now I wonder if > this is caused by that locking scheme (in a way I have not figured out yet) > or if it is a different issue. stmmac_tx_err races with stmmac_xmit. -- Ueimor

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On 08.12.2016 23:18, Pavel Machek wrote: > On Thu 2016-12-08 23:12:10, Lino Sanfilippo wrote: >> Hi, >> >> On 08.12.2016 22:54, Pavel Machek wrote: >> > On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote: >> >> Hi, >> >> >> >> On 08.12.2016 00:15, Francois Romieu wrote: >> >> > Lino Sanfilippo

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On Thu 2016-12-08 23:12:10, Lino Sanfilippo wrote: > Hi, > > On 08.12.2016 22:54, Pavel Machek wrote: > > On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote: > >> Hi, > >> > >> On 08.12.2016 00:15, Francois Romieu wrote: > >> > Lino Sanfilippo : > >> >> The driver uses a

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi, On 08.12.2016 22:54, Pavel Machek wrote: > On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote: >> Hi, >> >> On 08.12.2016 00:15, Francois Romieu wrote: >> > Lino Sanfilippo : >> >> The driver uses a private lock for synchronization between the xmit >> >> function and

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

On Thu 2016-12-08 21:32:12, Lino Sanfilippo wrote: > Hi, > > On 08.12.2016 00:15, Francois Romieu wrote: > > Lino Sanfilippo : > >> The driver uses a private lock for synchronization between the xmit > >> function and the xmit completion handler, but since the NETIF_F_LLTX

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Hi, On 08.12.2016 00:15, Francois Romieu wrote: > Lino Sanfilippo : >> The driver uses a private lock for synchronization between the xmit >> function and the xmit completion handler, but since the NETIF_F_LLTX flag >> is not set, the xmit function is also called with the

Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

Lino Sanfilippo : > The driver uses a private lock for synchronization between the xmit > function and the xmit completion handler, but since the NETIF_F_LLTX flag > is not set, the xmit function is also called with the xmit_lock held. > > On the other hand the xmit

[PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock

The driver uses a private lock for synchronization between the xmit function and the xmit completion handler, but since the NETIF_F_LLTX flag is not set, the xmit function is also called with the xmit_lock held. On the other hand the xmit completion handler first takes the private lock and (in