On Fri 2016-12-02 15:05:21, Lino Sanfilippo wrote: > Hi, > > > > > > There's nothing that protect stmmac_poll() from running concurently > > with stmmac_dma_interrupt(), right? > > > > could it be that there is also another issue concerned locking?: > The tx completion handler takes the xmit_lock in case that the > netif_queue is stopped. This is AFAICS unnecessary, since both > xmit and completion handler are already synchronized by the private > tx lock. But it is IMHO also dangerous: > > In the xmit handler we have the locking order > 1. xmit_lock > 2. private tx lock > > while in the completion handler its the reverse: > > 1. private tx lock > 2. xmit lock. > > Do I miss something?
No, it seems you are right. Something like this?
Hmm. And can priv->tx_lock be removed, as we already rely on
netif_tx_lock?
(I copied the "lock already held" annotations from forcedeth. I hope
they are right....)
Best regards,
Pavel
commit a6f21255dfc11fcadc5062dfd0c5f3d77ca4f634
Author: Pavel <pa...@ucw.cz>
Date: Wed Dec 7 13:29:15 2016 +0100
Reported-by: Lino Sanfilippo <linosanfili...@gmx.de>
The tx completion handler takes the xmit_lock in case that the
netif_queue is stopped. This is AFAICS unnecessary, since both
xmit and completion handler are already synchronized by the private
tx lock. But it is IMHO also dangerous:
In the xmit handler we have the locking order
1. xmit_lock
2. private tx lock
while in the completion handler its the reverse:
1. private tx lock
2. xmit lock.
Signed-off-by: Pavel Machek <pa...@denx.de>
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 982c952..5df9bb3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1380,14 +1380,9 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
if (unlikely(netif_queue_stopped(priv->dev) &&
stmmac_tx_avail(priv) > STMMAC_TX_THRESH)) {
- netif_tx_lock(priv->dev);
- if (netif_queue_stopped(priv->dev) &&
- stmmac_tx_avail(priv) > STMMAC_TX_THRESH) {
- netif_dbg(priv, tx_done, priv->dev,
- "%s: restart transmit\n", __func__);
- netif_wake_queue(priv->dev);
- }
- netif_tx_unlock(priv->dev);
+ netif_dbg(priv, tx_done, priv->dev,
+ "%s: restart transmit\n", __func__);
+ netif_wake_queue(priv->dev);
}
if ((priv->eee_enabled) && (!priv->tx_path_in_lpi_mode)) {
@@ -1630,7 +1625,9 @@ static void stmmac_tx_timer(unsigned long data)
{
struct stmmac_priv *priv = (struct stmmac_priv *)data;
+ netif_tx_lock_bh(priv->dev);
stmmac_tx_clean(priv);
+ netif_tx_unlock_bh(priv->dev);
}
/**
@@ -1994,7 +1991,8 @@ static void stmmac_tso_allocator(struct stmmac_priv
*priv, unsigned int des,
* --------
*
* mss is fixed when enable tso, so w/o programming the TDES3 ctx field.
- */
+ *
+ * Called with netif_tx_lock held.
*/
static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
{
u32 pay_len, mss;
@@ -2174,6 +2172,7 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb,
struct net_device *dev)
* Description : this is the tx entry point of the driver.
* It programs the chain or the ring and supports oversized frames
* and SG feature.
+ * Called with netif_tx_lock held.
*/
static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
{
@@ -2684,7 +2683,9 @@ static int stmmac_poll(struct napi_struct *napi, int
budget)
int work_done = 0;
priv->xstats.napi_poll++;
+ netif_tx_lock_bh(priv->dev);
stmmac_tx_clean(priv);
+ netif_tx_unlock_bh(priv->dev);
work_done = stmmac_rx(priv, budget);
if (work_done < budget) {
@@ -2701,6 +2702,7 @@ static int stmmac_poll(struct napi_struct *napi, int
budget)
* complete within a reasonable time. The driver will mark the error in the
* netdev structure and arrange for the device to be reset to a sane state
* in order to transmit a new packet.
+ * Called with netif_tx_lock held.
*/
static void stmmac_tx_timeout(struct net_device *dev)
{
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
