All of lore.kernel.org
 help / color / mirror / Atom feed
From: Pavel Machek <pavel@ucw.cz>
To: Joao Pinto <Joao.Pinto@synopsys.com>
Cc: Niklas Cassel <niklas.cassel@axis.com>,
	Giuseppe CAVALLARO <peppe.cavallaro@st.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	David Miller <davem@davemloft.net>,
	larper@axis.com, rabinv@axis.com, netdev <netdev@vger.kernel.org>,
	CARLOS.PALMINHA@synopsys.com, Jie.Deng1@synopsys.com,
	Stephen Warren <swarren@nvidia.com>
Subject: Re: Synopsys Ethernet QoS
Date: Sat, 17 Dec 2016 18:38:24 +0100	[thread overview]
Message-ID: <20161217173824.GB20231@amd> (raw)
In-Reply-To: <79642215-95ce-7f04-3db7-121c585e2f2a@synopsys.com>

[-- Attachment #1: Type: text/plain, Size: 3796 bytes --]

Hi!

> >> So if there is a long time before handling interrupts,
> >> I guess that it makes sense that one stream could
> >> get an advantage in the net scheduler.
> >>
> >> If I find the time, and if no one beats me to it, I will try to replace
> >> the normal timers with HR timers + a smaller default timeout.
> >>
> > 
> > Can you try something like this? Highres timers will be needed, too,
> > but this fixes the logic problem.
> > 
> > You'll need to apply it twice as code is copy&pasted.
> > 
> > Best regards,
> > 									Pavel
> > 
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > 
> >  	 */
> >  	priv->tx_count_frames += nfrags + 1;
> >  	if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> > -		mod_timer(&priv->txtimer,
> > -			  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> > +		if (priv->tx_count_frames == nfrags + 1)
> > +			mod_timer(&priv->txtimer,
> > +				  STMMAC_COAL_TIMER(priv->tx_coal_timer));
> >  	} else {
> >  		priv->tx_count_frames = 0;
> >  		priv->hw->desc->set_tx_ic(desc);
> > 
> > 
> 
> I know that this is completely of topic, but I am facing a dificulty with
> stmmac. I have interrupts, mac well configured rx packets being received
> successfully, but TX is not working, resulting in Tx errors = Total TX packets.
> I have made a lot of debug and my conclusions is that by some reason when using
> stmmac after starting tx dma, the hw state machine enters a deadend state
> resulting in those errors. Anyone faced this trouble?

SMP or UP system? AFAICT the driver gets the memory barriers wrong. It
does not fail completely for me, but still fails rather quickly.

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e40578..641b03d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2121,11 +2205,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (mss_desc)
 		priv->hw->desc->set_tx_owner(mss_desc);
 
-	/* The own bit must be the latest setting done when prepare the
+	/* The own bit must be the latest setting done when preparing the
 	 * descriptor and then barrier is needed to make sure that
 	 * all is coherent before granting the DMA engine.
 	 */
-	smp_wmb();
+	wmb();
 
 	if (netif_msg_pktdata(priv)) {
 		pr_info("%s: curr=%d dirty=%d f=%d, e=%d, f_p=%p, nfrags %d\n",
@@ -2336,9 +2401,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 
 		/* The own bit must be the latest setting done when prepare the
 		 * descriptor and then barrier is needed to make sure that
-		 * all is coherent before granting the DMA engine.
+		 * all is coherent before granting access to the DMA engine.
 		 */
-		smp_wmb();
+		wmb();
 	}
 
 	netdev_sent_queue(dev, skb->len);

Plus I'd suggest... at least (hand-edited). Driver should really be
modified to use readl() when accessing memory that changes.

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3e40578..641b03d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1309,6 +1323,8 @@ static void stmmac_tx_clean(struct stmmac_priv *priv)
 		status = priv->hw->desc->tx_status(&priv->dev->stats,
 						      &priv->xstats, p,
 						      priv->ioaddr);
+		rmb();
+		
 		/* Check if the descriptor is owned by the DMA */
 		if (unlikely(status & tx_dma_own))
 			break;

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

  parent reply	other threads:[~2016-12-17 17:38 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-09 11:29 Synopsys Ethernet QoS Joao Pinto
2016-12-09 15:33 ` David Miller
2016-12-09 15:36   ` Joao Pinto
2016-12-09 15:41     ` David Miller
2016-12-09 15:54       ` Joao Pinto
2016-12-09 22:25       ` Andy Shevchenko
2016-12-09 22:52         ` Florian Fainelli
2016-12-10  0:16           ` Andy Shevchenko
2016-12-10  1:44             ` Florian Fainelli
2016-12-12 10:19               ` Joao Pinto
2016-12-12 14:11                 ` Giuseppe CAVALLARO
2016-12-12 16:25                 ` Niklas Cassel
2016-12-12 16:46                   ` Stephen Warren
2016-12-13  7:22                   ` Giuseppe CAVALLARO
2016-12-13  9:38                     ` Niklas Cassel
2016-12-13  9:51                       ` Giuseppe CAVALLARO
2016-12-14 12:57                       ` Pavel Machek
2016-12-14 13:14                         ` Joao Pinto
2016-12-14 19:01                           ` stmmac: lockups (was Re: Synopsys Ethernet QoS) Pavel Machek
2016-12-15 11:09                           ` Synopsys Ethernet QoS Pavel Machek
2016-12-17 17:38                           ` Pavel Machek [this message]
2016-12-19 10:55                             ` Joao Pinto
2016-12-19 11:01                               ` Joao Pinto
2016-12-15 12:05                         ` Niklas Cassel
2016-12-19 17:58                         ` Niklas Cassel
2016-12-13 11:49                   ` Joao Pinto
2016-12-13 12:31                     ` Niklas Cassel
2016-12-13 12:50                       ` Lars Persson
2016-12-13 12:56                         ` Joao Pinto
2016-12-19 16:05                           ` Niklas Cassel
2016-12-14 11:54                   ` Pavel Machek
2016-12-10  2:13             ` Jie Deng

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161217173824.GB20231@amd \
    --to=pavel@ucw.cz \
    --cc=CARLOS.PALMINHA@synopsys.com \
    --cc=Jie.Deng1@synopsys.com \
    --cc=Joao.Pinto@synopsys.com \
    --cc=andy.shevchenko@gmail.com \
    --cc=davem@davemloft.net \
    --cc=f.fainelli@gmail.com \
    --cc=larper@axis.com \
    --cc=netdev@vger.kernel.org \
    --cc=niklas.cassel@axis.com \
    --cc=peppe.cavallaro@st.com \
    --cc=rabinv@axis.com \
    --cc=swarren@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.