| From dce84fe3415c3da2d71c4fedcf1fd90307647660 Mon Sep 17 00:00:00 2001 |
| From: Vladimir Oltean <olteanv@gmail.com> |
| Date: Sat, 28 Dec 2019 15:30:45 +0200 |
| Subject: [PATCH] gianfar: Fix TX timestamping with a stacked DSA driver |
| |
| commit c26a2c2ddc0115eb088873f5c309cf46b982f522 upstream. |
| |
| The driver wrongly assumes that it is the only entity that can set the |
| SKBTX_IN_PROGRESS bit of the current skb. Therefore, in the |
| gfar_clean_tx_ring function, where the TX timestamp is collected if |
| necessary, the aforementioned bit is used to discriminate whether or not |
| the TX timestamp should be delivered to the socket's error queue. |
| |
| But a stacked driver such as a DSA switch can also set the |
| SKBTX_IN_PROGRESS bit, which is actually exactly what it should do in |
| order to denote that the hardware timestamping process is undergoing. |
| |
| Therefore, gianfar would misinterpret the "in progress" bit as being its |
| own, and deliver a second skb clone in the socket's error queue, |
| completely throwing off a PTP process which is not expecting to receive |
| it, _even though_ TX timestamping is not enabled for gianfar. |
| |
| There have been discussions [0] as to whether non-MAC drivers need or |
| not to set SKBTX_IN_PROGRESS at all (whose purpose is to avoid sending 2 |
| timestamps, a sw and a hw one, to applications which only expect one). |
| But as of this patch, there are at least 2 PTP drivers that would break |
| in conjunction with gianfar: the sja1105 DSA switch and the felix |
| switch, by way of its ocelot core driver. |
| |
| So regardless of that conclusion, fix the gianfar driver to not do stuff |
| based on flags set by others and not intended for it. |
| |
| [0]: https://www.spinics.net/lists/netdev/msg619699.html |
| |
| Fixes: f0ee7acfcdd4 ("gianfar: Add hardware TX timestamping support") |
| Signed-off-by: Vladimir Oltean <olteanv@gmail.com> |
| Acked-by: Richard Cochran <richardcochran@gmail.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c |
| index 7ea19e173339..17963f95ae58 100644 |
| --- a/drivers/net/ethernet/freescale/gianfar.c |
| +++ b/drivers/net/ethernet/freescale/gianfar.c |
| @@ -2686,13 +2686,17 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) |
| skb_dirtytx = tx_queue->skb_dirtytx; |
| |
| while ((skb = tx_queue->tx_skbuff[skb_dirtytx])) { |
| + bool do_tstamp; |
| + |
| + do_tstamp = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && |
| + priv->hwts_tx_en; |
| |
| frags = skb_shinfo(skb)->nr_frags; |
| |
| /* When time stamping, one additional TxBD must be freed. |
| * Also, we need to dma_unmap_single() the TxPAL. |
| */ |
| - if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) |
| + if (unlikely(do_tstamp)) |
| nr_txbds = frags + 2; |
| else |
| nr_txbds = frags + 1; |
| @@ -2706,7 +2710,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) |
| (lstatus & BD_LENGTH_MASK)) |
| break; |
| |
| - if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) { |
| + if (unlikely(do_tstamp)) { |
| next = next_txbd(bdp, base, tx_ring_size); |
| buflen = be16_to_cpu(next->length) + |
| GMAC_FCB_LEN + GMAC_TXPAL_LEN; |
| @@ -2716,7 +2720,7 @@ static void gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) |
| dma_unmap_single(priv->dev, be32_to_cpu(bdp->bufPtr), |
| buflen, DMA_TO_DEVICE); |
| |
| - if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) { |
| + if (unlikely(do_tstamp)) { |
| struct skb_shared_hwtstamps shhwtstamps; |
| u64 *ns = (u64 *)(((uintptr_t)skb->data + 0x10) & |
| ~0x7UL); |
| -- |
| 2.7.4 |
| |