| Message ID | 20161124110548.GB30723@amd |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > Remove duplicate code from _tx routines. trivia: > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c [] > @@ -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__);
On Thu 2016-11-24 12:05:25, Joe Perches wrote: > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > > Remove duplicate code from _tx routines. > > trivia: > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > [] > > @@ -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. (And it looks like the driver has rather more serious problems than printk style...) Thanks, Pavel
On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote: > On Thu 2016-11-24 12:05:25, Joe Perches wrote: > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > > > Remove duplicate code from _tx routines. > > > > trivia: > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > [] > > > @@ -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. > (And it looks like the driver has rather more serious problems than > printk style...) Probably so. cheers, Joe
On Thu 2016-11-24 14:27:13, Joe Perches wrote: > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote: > > On Thu 2016-11-24 12:05:25, Joe Perches wrote: > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > > > > Remove duplicate code from _tx routines. > > > > > > trivia: > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > [] > > > > @@ -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. Feel free to do your favourite cleanup on whole tree, or per-driver basis. Doing it on per-message basis would be wrong thing to do. Thanks, Pavel
On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote: > On Thu 2016-11-24 14:27:13, Joe Perches wrote: > > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote: > > > On Thu 2016-11-24 12:05:25, Joe Perches wrote: > > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > > > > > Remove duplicate code from _tx routines. > > > > > > > > trivia: > > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > > > [] > > > > > @@ -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. cheers, Joe
On Mon 2016-11-28 06:24:28, Joe Perches wrote: > On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote: > > On Thu 2016-11-24 14:27:13, Joe Perches wrote: > > > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote: > > > > On Thu 2016-11-24 12:05:25, Joe Perches wrote: > > > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > > > > > > Remove duplicate code from _tx routines. > > > > > > > > > > trivia: > > > > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > > > > > [] > > > > > > @@ -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
On Mon, 2016-11-28 at 15:35 +0100, Pavel Machek wrote: > On Mon 2016-11-28 06:24:28, Joe Perches wrote: > > On Mon, 2016-11-28 at 12:50 +0100, Pavel Machek wrote: > > > On Thu 2016-11-24 14:27:13, Joe Perches wrote: > > > > On Thu, 2016-11-24 at 22:44 +0100, Pavel Machek wrote: > > > > > On Thu 2016-11-24 12:05:25, Joe Perches wrote: > > > > > > On Thu, 2016-11-24 at 12:05 +0100, Pavel Machek wrote: > > > > > > > Remove duplicate code from _tx routines. > > > > > > > > > > > > trivia: > > > > > > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > > > > > > > > [] > > > > > > > @@ -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? Yes and I think you should make changes against -next and not Linus' where this is: b3e51069627e2 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (LABBE Corentin 2016-11-16 20:09:41 +0100 755) netif_warn(priv, link, priv->dev, b3e51069627e2 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (LABBE Corentin 2016-11-16 20:09:41 +0100 756) "Speed (%d) not 10/100\n", b3e51069627e2 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c (LABBE Corentin 2016-11-16 20:09:41 +0100 757) phydev->speed); > 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. cheers, Joe
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 1cff258..5cf9cef 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -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_stop_queue(dev); + } + + dev->stats.tx_bytes += skb->len; + + /* According to the coalesce parameter the IC bit for the latest + * segment is reset and the timer re-started to clean the tx status. + * This approach takes care about the fragments: desc is the first + * element in case of no SG. + */ + 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)); + } else { + priv->tx_count_frames = 0; + priv->hw->desc->set_tx_ic(desc); + priv->xstats.tx_set_ic_bit++; + } + + if (!priv->hwts_tx_en) + skb_tx_timestamp(skb); +} + + /** * stmmac_tso_xmit - Tx entry point of the driver for oversized frames (TSO) * @skb : the socket buffer @@ -2081,30 +2113,11 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev) priv->cur_tx = STMMAC_GET_ENTRY(priv->cur_tx, DMA_TX_SIZE); - if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) { - if (netif_msg_hw(priv)) - pr_debug("%s: stop transmitted packets\n", __func__); - netif_stop_queue(dev); - } - - dev->stats.tx_bytes += skb->len; + stmmac_xmit_common(skb, dev, nfrags, desc); + priv->xstats.tx_tso_frames++; priv->xstats.tx_tso_nfrags += nfrags; - /* Manage tx mitigation */ - 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)); - } else { - priv->tx_count_frames = 0; - priv->hw->desc->set_tx_ic(desc); - priv->xstats.tx_set_ic_bit++; - } - - if (!priv->hwts_tx_en) - skb_tx_timestamp(skb); - if (unlikely((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && priv->hwts_tx_en)) { /* declare that device is doing timestamping */ @@ -2280,31 +2293,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev) print_pkt(skb->data, skb->len); } - if (unlikely(stmmac_tx_avail(priv) <= (MAX_SKB_FRAGS + 1))) { - if (netif_msg_hw(priv)) - pr_debug("%s: stop transmitted packets\n", __func__); - netif_stop_queue(dev); - } - - dev->stats.tx_bytes += skb->len; - - /* According to the coalesce parameter the IC bit for the latest - * segment is reset and the timer re-started to clean the tx status. - * This approach takes care about the fragments: desc is the first - * element in case of no SG. - */ - 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)); - } else { - priv->tx_count_frames = 0; - priv->hw->desc->set_tx_ic(desc); - priv->xstats.tx_set_ic_bit++; - } - - if (!priv->hwts_tx_en) - skb_tx_timestamp(skb); + stmmac_xmit_common(skb, dev, nfrags, desc); /* Ready to fill the first descriptor and set the OWN bit w/o any * problems because all the descriptors are actually ready to be
Remove duplicate code from _tx routines. Signed-off-by: Pavel Machek <pavel@denx.de>