Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540059 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-11 21:20 +0100
    Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-15 20:30 +0100
      Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-15 22:20 +0100
        Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-15 22:40 +0100
          Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-15 23:40 +0100
            Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-17 18:40 +0100
              Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Francois Romieu <romieu@fr.zoreil.com> - 2016-12-18 01:20 +0100
                Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-18 17:20 +0100
                  Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-18 18:30 +0100
                  Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-18 19:40 +0100
                    Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-12-19 23:50 +0100
                  Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-18 21:20 +0100
                Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Pavel Machek <pavel@ucw.cz> - 2016-12-19 11:10 +0100
                  Re: [PATCH 1/2] net: ethernet: sxgbe: remove private tx queue lock Francois Romieu <romieu@fr.zoreil.com> - 2016-12-20 01:10 +0100

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

[Multipart message — attachments visible in raw view] — view raw

Hi!

> On 09.12.2016 12:21, Pavel Machek wrote:
> > On Fri 2016-12-09 00:19:43, Francois Romieu wrote:
> >> Lino Sanfilippo <LinoSanfilippo@gmx.de> :
> >> [...]
> >> > 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.
> > 
> > Umm, yes, that looks real.
> > 
> > And that means that removing tx_lock will not be completely trivial
> > :-(. Lino, any ideas there?
> > 
> 
> Ok, the race is there but it looks like a problem that is not related to 
> the use or removal of the private lock.

Well, removal of the private lock will make it trickier to fix :-(.

> By a glimpse into other drivers (e.g sky2 or e1000), a possible way to handle a 
> tx error is to start a separate task and restart the tx path in that task instead
> the irq handler (or timer in case of the watchdog).
> 
> In that task we could do:
> 1. deactivate napi
> 2. deactivate irqs
> 3. wait for running napi/irqs do complete (_sync)
> 4. call stmmac_tx_err()
> 5. reenable napi
> 6. reenable irqs
> 
> We have to ensure that no xmit() is executing while stmmac_tx_err() does the cleanup,
> so stmmac_tx_err() should IMO rather call netif_tx_disable() instead of netif_stop_queue()
> (the former grabs the xmit lock before it sets __QUEUE_STATE_DRV_XOFF to disable
> the queue).

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?

Because it does not currently, and I don't know how to test that
code. Unplugging the cable does not provoke that.

I tried

        } else if (unlikely(status == tx_hard_error))
                stmmac_tx_err(priv);
+
+       {
+               static int i;
+               i++;
+               if (i==1000) {
+                       i = 0;
+                       printk("Simulated error\n");
+                       stmmac_tx_err(priv);
+               }
+       }
 }

but the driver does not survive that :-(.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1542993

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 errorneous tx path, e.g. a
missing tx completion for transmitted frames.

Some drivers do this by restarting only the HW parts responsible for tx, some
others by restarting the complete hardware. 
But IMO it should also be ok to be called if the HW is still working fine.

> Because it does not currently, and I don't know how to test that
> code. Unplugging the cable does not provoke that.
> 
> I tried
> 
>         } else if (unlikely(status == tx_hard_error))
>                 stmmac_tx_err(priv);
> +
> +       {
> +               static int i;
> +               i++;
> +               if (i==1000) {
> +                       i = 0;
> +                       printk("Simulated error\n");
> +                       stmmac_tx_err(priv);
> +               }
> +       }
>  }
> 

Ok, there is this race that Francois mentioned so it is not surprising that
the driver does not survive the call of stmmac_tx_err() as it is called now.
Thats why I suggested to do a proper shutdown and restart of the tx path to
avoid the race.

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1543074

[Multipart message — attachments visible in raw view] — view raw

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 far as I understood it is supposed to fixup an errorneous tx path, e.g. a
> missing tx completion for transmitted frames.
> 
> Some drivers do this by restarting only the HW parts responsible for tx, some
> others by restarting the complete hardware. 
> But IMO it should also be ok to be called if the HW is still working fine.
> 
> > Because it does not currently, and I don't know how to test that
> > code. Unplugging the cable does not provoke that.
> > 
> > I tried
> > 
> >         } else if (unlikely(status == tx_hard_error))
> >                 stmmac_tx_err(priv);
> > +
> > +       {
> > +               static int i;
> > +               i++;
> > +               if (i==1000) {
> > +                       i = 0;
> > +                       printk("Simulated error\n");
> > +                       stmmac_tx_err(priv);
> > +               }
> > +       }
> >  }
> > 
> 
> Ok, there is this race that Francois mentioned so it is not surprising that
> the driver does not survive the call of stmmac_tx_err() as it is called now.
> Thats why I suggested to do a proper shutdown and restart of the tx path to
> avoid the race.

I actually did experiment with adding locking there, too, and no, no
luck. It seems stmmac_tx_err() is more broken than just locking.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1543078

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 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) paths are stopped.
Did you try to stop the phy fist in stmmac_tx_err_work(), too?

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1543102

[Multipart message — attachments visible in raw view] — view raw

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) paths are stopped.
> Did you try to stop the phy fist in stmmac_tx_err_work(), too?
> 
> Regards,
> Lino
> 

And this is the "sledgehammer" approach: Do a complete shutdown and restart
of the hardware in case of tx error (against net-next and only compile tested).

[toc] | [prev] | [next] | [standalone]

#1544020

[Multipart message — attachments visible in raw view] — view raw

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. OTOH stmmac_release() also stops the phy before the tx (and rx) paths are stopped.
> > Did you try to stop the phy fist in stmmac_tx_err_work(), too?
> > 
> > Regards,
> > Lino
> > 
> 
> And this is the "sledgehammer" approach: Do a complete shutdown and restart
> of the hardware in case of tx error (against net-next and only
>compile tested).

Wow, thanks a lot. I'll try to get the driver back to the non-working
state, and try it.

I believe I have some idea what is wrong there. (Missing memory barriers).

> +static void stmmac_tx_err_work(struct work_struct *work)
> +{
> +	struct stmmac_priv *priv = container_of(work, struct stmmac_priv,
> +						tx_err_work);
> +	/* restart netdev */
> +	rtnl_lock();
> +	stmmac_release(priv->dev);
> +	stmmac_open(priv->dev);
> +	rtnl_unlock();
> +}

Won't this up/down the interface, in a way userspace can observe?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1544056

Pavel Machek <pavel@ucw.cz> :
[...]
> 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 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 avoidable) extra error paths due to stmmac_release()
2) racy cancel_work_sync. Low probability as it is, an irq + error could
   take place right after cancel_work_sync

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. Thought / opinion ?

[*] Including fixes/changes in:
- 3a5a883a8a663b930908cae4abe5ec913b9b2fd2
- e1efa87241272104d6a12c8b9fcdc4f62634d447
- 810f19bcb862f8889b27e0c9d9eceac9593925dd
- e45af497950a89459a0c4b13ffd91e1729fffef4
- a926592f5e4e900f3fa903298c4619a131e60963
- 559bcac35facfed49ab4f408e162971612dcfdf3

-- 
Ueimor

[toc] | [prev] | [next] | [standalone]

#1544162

Hi,

On 18.12.2016 01:15, Francois Romieu wrote:
> Pavel Machek <pavel@ucw.cz> :
> [...]
>> 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. Userspace wont see link down/up, but it will see carrier off/on.
But this is AFAIK acceptable for a rare situation like a tx error.

> 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 avoidable) extra error paths due to stmmac_release()
> 2) racy cancel_work_sync. Low probability as it is, an irq + error could
>    take place right after cancel_work_sync

It was indeed only meant as a proof of concept. Nevertheless the race is not 
good, since one can run into it when faking the tx error for testings purposes.
So below is a slightly improved version of the restart handling.
Its not meant as a final version either. But maybe we can use it as a starting
point.

> 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. Thought / opinion ?
> 

Not really. There are a few drivers that I use to look into if I want to know
how certain things are done correctly (e.g the sky2 or the intel drivers) because
I think they are well implemented.
But you seem to have put some thoughts into various race condition problems
in the via-rhine driver and I can image that sxgbe and stmmac still have some
of these issues, too.

> [*] Including fixes/changes in:
> - 3a5a883a8a663b930908cae4abe5ec913b9b2fd2

Ok, the issues you fixed here are concerning the tx_curr and tx_dirty
pointers. For now this is not needed in stmmac and sxgbe since the
tx completion handlers in both drivers are not lock-free like in 
the via-rhine.c but are synchronized with xmit by means of the xmit_lock.

> - e1efa87241272104d6a12c8b9fcdc4f62634d447

Yep, a sync of the dma descriptors before the hardware gets ownership of the tx tail
idx is missing in the stmmac, too. 

> - 810f19bcb862f8889b27e0c9d9eceac9593925dd
> - e45af497950a89459a0c4b13ffd91e1729fffef4
> - a926592f5e4e900f3fa903298c4619a131e60963

I think we should use netif_tx_disable() instead of netif_stop_queue(), too, in 
case of restart to avoid a possible schedule of the xmit function while we restart. 
So this is also part of the new patch.

Again the patch is only compile tested.

Regards,
Lino

---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  1 +
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 95 +++++++++++++++--------
 2 files changed, 63 insertions(+), 33 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index eab04ae..9c240d7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -131,6 +131,7 @@ struct stmmac_priv {
 	u32 rx_tail_addr;
 	u32 tx_tail_addr;
 	u32 mss;
+	struct work_struct tx_err_work;
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *dbgfs_dir;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e40578..5762750 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1403,37 +1403,6 @@ static inline void stmmac_disable_dma_irq(struct stmmac_priv *priv)
 }
 
 /**
- * stmmac_tx_err - to manage the tx error
- * @priv: driver private structure
- * Description: it cleans the descriptors and restarts the transmission
- * in case of transmission errors.
- */
-static void stmmac_tx_err(struct stmmac_priv *priv)
-{
-	int i;
-	netif_stop_queue(priv->dev);
-
-	priv->hw->dma->stop_tx(priv->ioaddr);
-	dma_free_tx_skbufs(priv);
-	for (i = 0; i < DMA_TX_SIZE; i++)
-		if (priv->extend_desc)
-			priv->hw->desc->init_tx_desc(&priv->dma_etx[i].basic,
-						     priv->mode,
-						     (i == DMA_TX_SIZE - 1));
-		else
-			priv->hw->desc->init_tx_desc(&priv->dma_tx[i],
-						     priv->mode,
-						     (i == DMA_TX_SIZE - 1));
-	priv->dirty_tx = 0;
-	priv->cur_tx = 0;
-	netdev_reset_queue(priv->dev);
-	priv->hw->dma->start_tx(priv->ioaddr);
-
-	priv->dev->stats.tx_errors++;
-	netif_wake_queue(priv->dev);
-}
-
-/**
  * stmmac_dma_interrupt - DMA ISR
  * @priv: driver private structure
  * Description: this is the DMA ISR. It is called by the main ISR.
@@ -1466,7 +1435,7 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
 			priv->xstats.threshold = tc;
 		}
 	} else if (unlikely(status == tx_hard_error))
-		stmmac_tx_err(priv);
+		schedule_work(&priv->tx_err_work);
 }
 
 /**
@@ -1902,6 +1871,7 @@ static int stmmac_release(struct net_device *dev)
 	if (priv->lpi_irq > 0)
 		free_irq(priv->lpi_irq, dev);
 
+	cancel_work_sync(&priv->tx_err_work);
 	/* Stop TX/RX DMA and clear the descriptors */
 	priv->hw->dma->stop_tx(priv->ioaddr);
 	priv->hw->dma->stop_rx(priv->ioaddr);
@@ -1920,9 +1890,67 @@ static int stmmac_release(struct net_device *dev)
 
 	stmmac_release_ptp(priv);
 
+
 	return 0;
 }
 
+static void stmmac_shutdown(struct net_device *dev)
+{
+	struct stmmac_priv *priv = netdev_priv(dev);
+
+	/* make sure xmit is not scheduled any more */
+	netif_tx_disable(dev);
+
+	if (priv->eee_enabled)
+		del_timer_sync(&priv->eee_ctrl_timer);
+
+	/* Stop and disconnect the PHY */
+	if (dev->phydev) {
+		phy_stop(dev->phydev);
+		phy_disconnect(dev->phydev);
+	}
+
+	napi_disable(&priv->napi);
+
+	del_timer_sync(&priv->txtimer);
+
+	/* Free the IRQ lines */
+	free_irq(dev->irq, dev);
+	if (priv->wol_irq != dev->irq)
+		free_irq(priv->wol_irq, dev);
+	if (priv->lpi_irq > 0)
+		free_irq(priv->lpi_irq, dev);
+
+	/* Stop TX/RX DMA and clear the descriptors */
+	priv->hw->dma->stop_tx(priv->ioaddr);
+	priv->hw->dma->stop_rx(priv->ioaddr);
+
+	/* Release and free the Rx/Tx resources */
+	free_dma_desc_resources(priv);
+
+	/* Disable the MAC Rx/Tx */
+	stmmac_set_mac(priv->ioaddr, false);
+
+	netif_carrier_off(dev);
+
+#ifdef CONFIG_DEBUG_FS
+	stmmac_exit_fs(dev);
+#endif
+
+	stmmac_release_ptp(priv);
+}
+
+static void stmmac_tx_err_work(struct work_struct *work)
+{
+	struct stmmac_priv *priv = container_of(work, struct stmmac_priv,
+						tx_err_work);
+	/* restart netdev */
+	rtnl_lock();
+	stmmac_shutdown(priv->dev);
+	stmmac_open(priv->dev);
+	rtnl_unlock();
+}
+
 /**
  *  stmmac_tso_allocator - close entry point of the driver
  *  @priv: driver private structure
@@ -2688,7 +2716,7 @@ static void stmmac_tx_timeout(struct net_device *dev)
 	struct stmmac_priv *priv = netdev_priv(dev);
 
 	/* Clear Tx resources and restart transmitting again */
-	stmmac_tx_err(priv);
+	schedule_work(&priv->tx_err_work);
 }
 
 /**
@@ -3338,6 +3366,7 @@ int stmmac_dvr_probe(struct device *device,
 	netif_napi_add(ndev, &priv->napi, stmmac_poll, 64);
 
 	spin_lock_init(&priv->lock);
+	INIT_WORK(&priv->tx_err_work, stmmac_tx_err_work);
 
 	ret = register_netdev(ndev);
 	if (ret) {
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1544178

[Multipart message — attachments visible in raw view] — view raw

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 clean_tx, too. Anyway,
I did not notice there are dma_ variants, too... we clearly need them.

Thanks and best regards,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1544184

[Multipart message — attachments visible in raw view] — view raw

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

[ 1018.410012] stmmac: early irq
[ 1023.939702] fpga_cmd_read:wait_event timed out!
[ 1033.128692] ------------[ cut here ]------------
[ 1033.133329] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:303
dev_watchdog+0x264/0x284()
[ 1033.141748] NETDEV WATCHDOG: eth0 (socfpga-dwmac): transmit queue 0
timed out
[ 1033.148861] Modules linked in:

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1544803

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 with dma_
> variant of barriers, and it failed, too
> 
> [ 1018.410012] stmmac: early irq
> [ 1023.939702] fpga_cmd_read:wait_event timed out!
> [ 1033.128692] ------------[ cut here ]------------
> [ 1033.133329] WARNING: CPU: 0 PID: 0 at net/sched/sch_generic.c:303
> dev_watchdog+0x264/0x284()
> [ 1033.141748] NETDEV WATCHDOG: eth0 (socfpga-dwmac): transmit queue 0
> timed out
> [ 1033.148861] Modules linked in:

This watchdog warning clearly says that for some reason the tx queue was stopped
but never woken up in a certain timespan (5 sec in our case, which is a lot).

Does this occur after the queue has been stopped and woken up again a few times or
is it already the first time the queue is stopped (and never woken up)? 

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1544199

[Multipart message — attachments visible in raw view] — view raw

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 avoidable) extra error paths due to stmmac_release()
> > 2) racy cancel_work_sync. Low probability as it is, an irq + error could
> >    take place right after cancel_work_sync
> 
> It was indeed only meant as a proof of concept. Nevertheless the race is not 
> good, since one can run into it when faking the tx error for testings purposes.
> So below is a slightly improved version of the restart handling.
> Its not meant as a final version either. But maybe we can use it as a starting
> point.

Certainly works better than version we currently have in tree. I'm
running it in a loop, and it survived 10 minutes of testing so
far. (Previous version killed the hardware at first iteration.)

> Again the patch is only compile tested.

Tested-by: Pavel Machek <pavel@denx.de>

Thanks!
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1544419

[Multipart message — attachments visible in raw view] — view raw

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. Thought / opinion ?
> 
> [*] Including fixes/changes in:
> - 3a5a883a8a663b930908cae4abe5ec913b9b2fd2
> - e1efa87241272104d6a12c8b9fcdc4f62634d447
> - 810f19bcb862f8889b27e0c9d9eceac9593925dd
> - e45af497950a89459a0c4b13ffd91e1729fffef4
> - a926592f5e4e900f3fa903298c4619a131e60963
> - 559bcac35facfed49ab4f408e162971612dcfdf3

Considering the memory barriers... is something like this neccessary
in the via-rhine?

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 we need a barrier before giving the descriptor to
the dma, to make sure DMA engine sees the complete update....?

Thanks,
								Pavel

diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
index ba5c542..3806e72 100644
--- a/drivers/net/ethernet/via/via-rhine.c
+++ b/drivers/net/ethernet/via/via-rhine.c
@@ -1952,6 +1952,7 @@ static void rhine_tx(struct net_device *dev)
 			  entry, txstatus);
 		if (txstatus & DescOwn)
 			break;
+		dma_rmb();
 		skb = rp->tx_skbuff[entry];
 		if (txstatus & 0x8000) {
 			netif_dbg(rp, tx_done, dev,
@@ -2061,6 +2062,7 @@ static int rhine_rx(struct net_device *dev, int limit)
 
 		if (desc_status & DescOwn)
 			break;
+		dma_rmb();
 
 		netif_dbg(rp, rx_status, dev, "%s() status %08x\n", __func__,
 			  desc_status);
@@ -2146,6 +2148,7 @@ static int rhine_rx(struct net_device *dev, int limit)
 			u64_stats_update_end(&rp->rx_stats.syncp);
 		}
 give_descriptor_to_nic:
+		dma_wmb();
 		desc->rx_status = cpu_to_le32(DescOwn);
 		entry = (++rp->cur_rx) % RX_RING_SIZE;
 	}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1544832

Pavel Machek <pavel@ucw.cz> :
[...]
> 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 we need a barrier before giving the descriptor to
> the dma, to make sure DMA engine sees the complete update....?

I would not expect stale data while processing a single transmit
descriptor as the transmit completion does not use the rest of the
descriptor at all in the via-rhine driver. However I agree that transmit
descriptors should be read by the cpu with adequate ordering so the
dma_rmb() should stay.

Same kind of narrative for dma_wmb rhine_rx (s/read/written/ and
s/cpu/device/).

> diff --git a/drivers/net/ethernet/via/via-rhine.c b/drivers/net/ethernet/via/via-rhine.c
> index ba5c542..3806e72 100644
> --- a/drivers/net/ethernet/via/via-rhine.c
> +++ b/drivers/net/ethernet/via/via-rhine.c
[...]
> @@ -2061,6 +2062,7 @@ static int rhine_rx(struct net_device *dev, int limit)
>  
>  		if (desc_status & DescOwn)
>  			break;
> +		dma_rmb();
>  

I agree with your explanation for this one (late vlan processing in a 
different word from the same descriptor).

-- 
Ueimor

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web