Re: [PATCH] stmmac ethernet: remove cut & paste code

On Mon 2016-11-28 06:24:28, Joe Perches wrote:

On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote:

quoted

On Thu 2016-11-24 14:27:13, Joe Perches wrote:

quoted

On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote:

quoted

On Thu 2016-11-24 12:05:25, Joe Perches wrote:

quoted

On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote:

quoted

Remove duplicate code from _tx routines.

trivia:

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[]

quoted

@@ -1960,6 +1960,38 @@ static void stmmac_tso_allocator(struct stmmac_priv *priv, unsigned int des,
 	}
 }
 
+static void stmmac_xmit_common(struct sk_buff *skb, struct net_device *dev, int nfrags, struct dma_desc *desc)
+{
+	struct stmmac_priv *priv = netdev_priv(dev);
+
+	if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) {
+		if (netif_msg_hw(priv))
+			pr_debug("%s: stop transmitted packets\n", __func__);

		netif_dbg(priv, hw, dev, "%s: stop transmitted packets\n",
			  __func__);

Not now. Modifying the code while de-duplicating would be bad idea.

Too many people think overly granular patches are the
best and only way to make changes.
Deduplication and consolidation can happen simultaneously.

Can, but should not at this point. Please take a look at the driver in
question before commenting on trivial printk style.

I had.

It's perfectly acceptable and already uses netif_<level> properly.

This consolidation now introduces the _only_ instance where it is
now improperly using a netif_msg_<type> then single pr_<level>
function sequence that should be consolidated into netif_dbg.

Every other use of netif_msg_<level> then either emits multiple
lines or is used in an if/else.

Are you looking at right driver? I don't see single use of
netif_msg_<level>, but see this at stmmac_main.c:756. Code is actually
pretty consistent using pr_*.

                                if (netif_msg_link(priv))
                                        pr_warn("%s: Speed (%d) not 10/100\n",
                                                dev->name, phydev->speed);

Anyway, I'm moving code around, if you want to do trivial cleanups, do
them yourself.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html