* [PATCH net-next] net: macb: simplify TX timestamp handling
@ 2023-01-16 22:08 Robert Hancock
2023-01-18 0:19 ` Jacob Keller
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Robert Hancock @ 2023-01-16 22:08 UTC (permalink / raw)
To: Nicolas Ferre, Claudiu Beznea, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Richard Cochran
Cc: Robert Hancock, netdev, linux-kernel
This driver was capturing the TX timestamp values from the TX ring
during the TX completion path, but deferring the actual packet TX
timestamp updating to a workqueue. There does not seem to be much of a
reason for this with the current state of the driver. Simplify this to
just do the TX timestamping as part of the TX completion path, to avoid
the need for the extra timestamp buffer and workqueue.
Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
drivers/net/ethernet/cadence/macb.h | 29 ++-------
drivers/net/ethernet/cadence/macb_main.c | 16 +++--
drivers/net/ethernet/cadence/macb_ptp.c | 83 ++++++------------------
3 files changed, 34 insertions(+), 94 deletions(-)
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 9c410f93a103..14dfec4db8f9 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -768,8 +768,6 @@
#define gem_readl_n(port, reg, idx) (port)->macb_reg_readl((port), GEM_##reg + idx * 4)
#define gem_writel_n(port, reg, idx, value) (port)->macb_reg_writel((port), GEM_##reg + idx * 4, (value))
-#define PTP_TS_BUFFER_SIZE 128 /* must be power of 2 */
-
/* Conditional GEM/MACB macros. These perform the operation to the correct
* register dependent on whether the device is a GEM or a MACB. For registers
* and bitfields that are common across both devices, use macb_{read,write}l
@@ -819,11 +817,6 @@ struct macb_dma_desc_ptp {
u32 ts_1;
u32 ts_2;
};
-
-struct gem_tx_ts {
- struct sk_buff *skb;
- struct macb_dma_desc_ptp desc_ptp;
-};
#endif
/* DMA descriptor bitfields */
@@ -1224,12 +1217,6 @@ struct macb_queue {
void *rx_buffers;
struct napi_struct napi_rx;
struct queue_stats stats;
-
-#ifdef CONFIG_MACB_USE_HWSTAMP
- struct work_struct tx_ts_task;
- unsigned int tx_ts_head, tx_ts_tail;
- struct gem_tx_ts tx_timestamps[PTP_TS_BUFFER_SIZE];
-#endif
};
struct ethtool_rx_fs_item {
@@ -1340,14 +1327,14 @@ enum macb_bd_control {
void gem_ptp_init(struct net_device *ndev);
void gem_ptp_remove(struct net_device *ndev);
-int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *des);
+void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
-static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
+static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
{
- if (queue->bp->tstamp_config.tx_type == TSTAMP_DISABLED)
- return -ENOTSUPP;
+ if (bp->tstamp_config.tx_type == TSTAMP_DISABLED)
+ return;
- return gem_ptp_txstamp(queue, skb, desc);
+ gem_ptp_txstamp(bp, skb, desc);
}
static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
@@ -1363,11 +1350,7 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
static inline void gem_ptp_init(struct net_device *ndev) { }
static inline void gem_ptp_remove(struct net_device *ndev) { }
-static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
-{
- return -1;
-}
-
+static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
#endif
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 95667b979fab..6a0419acac9d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1191,13 +1191,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
/* First, update TX stats if needed */
if (skb) {
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
- !ptp_one_step_sync(skb) &&
- gem_ptp_do_txstamp(queue, skb, desc) == 0) {
- /* skb now belongs to timestamp buffer
- * and will be removed later
- */
- tx_skb->skb = NULL;
- }
+ !ptp_one_step_sync(skb))
+ gem_ptp_do_txstamp(bp, skb, desc);
+
netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
macb_tx_ring_wrap(bp, tail),
skb->data);
@@ -2260,6 +2256,12 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
return ret;
}
+#ifdef CONFIG_MACB_USE_HWSTAMP
+ if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
+ (bp->hw_dma_cap & HW_DMA_CAP_PTP))
+ skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+#endif
+
is_lso = (skb_shinfo(skb)->gso_size != 0);
if (is_lso) {
diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
index e6cb20aaa76a..f962a95068a0 100644
--- a/drivers/net/ethernet/cadence/macb_ptp.c
+++ b/drivers/net/ethernet/cadence/macb_ptp.c
@@ -292,79 +292,39 @@ void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb,
}
}
-static void gem_tstamp_tx(struct macb *bp, struct sk_buff *skb,
- struct macb_dma_desc_ptp *desc_ptp)
+void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb,
+ struct macb_dma_desc *desc)
{
struct skb_shared_hwtstamps shhwtstamps;
- struct timespec64 ts;
-
- gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
- memset(&shhwtstamps, 0, sizeof(shhwtstamps));
- shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
- skb_tstamp_tx(skb, &shhwtstamps);
-}
-
-int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
- struct macb_dma_desc *desc)
-{
- unsigned long tail = READ_ONCE(queue->tx_ts_tail);
- unsigned long head = queue->tx_ts_head;
struct macb_dma_desc_ptp *desc_ptp;
- struct gem_tx_ts *tx_timestamp;
-
- if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
- return -EINVAL;
+ struct timespec64 ts;
- if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
- return -ENOMEM;
+ if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl)) {
+ dev_warn_ratelimited(&bp->pdev->dev,
+ "Timestamp not set in TX BD as expected\n");
+ return;
+ }
- desc_ptp = macb_ptp_desc(queue->bp, desc);
+ desc_ptp = macb_ptp_desc(bp, desc);
/* Unlikely but check */
- if (!desc_ptp)
- return -EINVAL;
- skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
- tx_timestamp = &queue->tx_timestamps[head];
- tx_timestamp->skb = skb;
+ if (!desc_ptp) {
+ dev_warn_ratelimited(&bp->pdev->dev,
+ "Timestamp not supported in BD\n");
+ return;
+ }
+
/* ensure ts_1/ts_2 is loaded after ctrl (TX_USED check) */
dma_rmb();
- tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
- tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
- /* move head */
- smp_store_release(&queue->tx_ts_head,
- (head + 1) & (PTP_TS_BUFFER_SIZE - 1));
-
- schedule_work(&queue->tx_ts_task);
- return 0;
-}
+ gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
-static void gem_tx_timestamp_flush(struct work_struct *work)
-{
- struct macb_queue *queue =
- container_of(work, struct macb_queue, tx_ts_task);
- unsigned long head, tail;
- struct gem_tx_ts *tx_ts;
-
- /* take current head */
- head = smp_load_acquire(&queue->tx_ts_head);
- tail = queue->tx_ts_tail;
-
- while (CIRC_CNT(head, tail, PTP_TS_BUFFER_SIZE)) {
- tx_ts = &queue->tx_timestamps[tail];
- gem_tstamp_tx(queue->bp, tx_ts->skb, &tx_ts->desc_ptp);
- /* cleanup */
- dev_kfree_skb_any(tx_ts->skb);
- /* remove old tail */
- smp_store_release(&queue->tx_ts_tail,
- (tail + 1) & (PTP_TS_BUFFER_SIZE - 1));
- tail = queue->tx_ts_tail;
- }
+ memset(&shhwtstamps, 0, sizeof(shhwtstamps));
+ shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
+ skb_tstamp_tx(skb, &shhwtstamps);
}
void gem_ptp_init(struct net_device *dev)
{
struct macb *bp = netdev_priv(dev);
- struct macb_queue *queue;
- unsigned int q;
bp->ptp_clock_info = gem_ptp_caps_template;
@@ -384,11 +344,6 @@ void gem_ptp_init(struct net_device *dev)
}
spin_lock_init(&bp->tsu_clk_lock);
- for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
- queue->tx_ts_head = 0;
- queue->tx_ts_tail = 0;
- INIT_WORK(&queue->tx_ts_task, gem_tx_timestamp_flush);
- }
gem_ptp_init_tsu(bp);
--
2.39.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: macb: simplify TX timestamp handling
2023-01-16 22:08 [PATCH net-next] net: macb: simplify TX timestamp handling Robert Hancock
@ 2023-01-18 0:19 ` Jacob Keller
2023-01-18 10:24 ` Claudiu.Beznea
2023-01-18 14:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Jacob Keller @ 2023-01-18 0:19 UTC (permalink / raw)
To: Robert Hancock, Nicolas Ferre, Claudiu Beznea, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran
Cc: netdev, linux-kernel
On 1/16/2023 2:08 PM, Robert Hancock wrote:
> This driver was capturing the TX timestamp values from the TX ring
> during the TX completion path, but deferring the actual packet TX
> timestamp updating to a workqueue. There does not seem to be much of a
> reason for this with the current state of the driver. Simplify this to
> just do the TX timestamping as part of the TX completion path, to avoid
> the need for the extra timestamp buffer and workqueue.
>
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
Makes sense. I can't see anything that would require a work queue, and
removing that is a big win. (It's caused no end of troubles in some of
our Intel drivers, but we have no choice due to the hardware design :( )
I'm not super familiar with this code, but..
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> drivers/net/ethernet/cadence/macb.h | 29 ++-------
> drivers/net/ethernet/cadence/macb_main.c | 16 +++--
> drivers/net/ethernet/cadence/macb_ptp.c | 83 ++++++------------------
> 3 files changed, 34 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 9c410f93a103..14dfec4db8f9 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -768,8 +768,6 @@
> #define gem_readl_n(port, reg, idx) (port)->macb_reg_readl((port), GEM_##reg + idx * 4)
> #define gem_writel_n(port, reg, idx, value) (port)->macb_reg_writel((port), GEM_##reg + idx * 4, (value))
>
> -#define PTP_TS_BUFFER_SIZE 128 /* must be power of 2 */
> -
> /* Conditional GEM/MACB macros. These perform the operation to the correct
> * register dependent on whether the device is a GEM or a MACB. For registers
> * and bitfields that are common across both devices, use macb_{read,write}l
> @@ -819,11 +817,6 @@ struct macb_dma_desc_ptp {
> u32 ts_1;
> u32 ts_2;
> };
> -
> -struct gem_tx_ts {
> - struct sk_buff *skb;
> - struct macb_dma_desc_ptp desc_ptp;
> -};
> #endif
>
> /* DMA descriptor bitfields */
> @@ -1224,12 +1217,6 @@ struct macb_queue {
> void *rx_buffers;
> struct napi_struct napi_rx;
> struct queue_stats stats;
> -
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> - struct work_struct tx_ts_task;
> - unsigned int tx_ts_head, tx_ts_tail;
> - struct gem_tx_ts tx_timestamps[PTP_TS_BUFFER_SIZE];
> -#endif
> };
>
> struct ethtool_rx_fs_item {
> @@ -1340,14 +1327,14 @@ enum macb_bd_control {
>
> void gem_ptp_init(struct net_device *ndev);
> void gem_ptp_remove(struct net_device *ndev);
> -int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *des);
> +void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
> void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
> -static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
> +static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
> {
> - if (queue->bp->tstamp_config.tx_type == TSTAMP_DISABLED)
> - return -ENOTSUPP;
> + if (bp->tstamp_config.tx_type == TSTAMP_DISABLED)
> + return;
>
> - return gem_ptp_txstamp(queue, skb, desc);
> + gem_ptp_txstamp(bp, skb, desc);
> }
>
> static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
> @@ -1363,11 +1350,7 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
> static inline void gem_ptp_init(struct net_device *ndev) { }
> static inline void gem_ptp_remove(struct net_device *ndev) { }
>
> -static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
> -{
> - return -1;
> -}
> -
> +static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
> static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
> #endif
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 95667b979fab..6a0419acac9d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1191,13 +1191,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> /* First, update TX stats if needed */
> if (skb) {
> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> - !ptp_one_step_sync(skb) &&
> - gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> - /* skb now belongs to timestamp buffer
> - * and will be removed later
> - */
> - tx_skb->skb = NULL;
> - }
> + !ptp_one_step_sync(skb))
> + gem_ptp_do_txstamp(bp, skb, desc);
> +
> netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
> macb_tx_ring_wrap(bp, tail),
> skb->data);
> @@ -2260,6 +2256,12 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
> return ret;
> }
>
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> + (bp->hw_dma_cap & HW_DMA_CAP_PTP))
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +#endif
> +
> is_lso = (skb_shinfo(skb)->gso_size != 0);
>
> if (is_lso) {
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index e6cb20aaa76a..f962a95068a0 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -292,79 +292,39 @@ void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb,
> }
> }
>
> -static void gem_tstamp_tx(struct macb *bp, struct sk_buff *skb,
> - struct macb_dma_desc_ptp *desc_ptp)
> +void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb,
> + struct macb_dma_desc *desc)
> {
> struct skb_shared_hwtstamps shhwtstamps;
> - struct timespec64 ts;
> -
> - gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
> - memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> - shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> - skb_tstamp_tx(skb, &shhwtstamps);
> -}
> -
> -int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
> - struct macb_dma_desc *desc)
> -{
> - unsigned long tail = READ_ONCE(queue->tx_ts_tail);
> - unsigned long head = queue->tx_ts_head;
> struct macb_dma_desc_ptp *desc_ptp;
> - struct gem_tx_ts *tx_timestamp;
> -
> - if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
> - return -EINVAL;
> + struct timespec64 ts;
>
> - if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
> - return -ENOMEM;
> + if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl)) {
> + dev_warn_ratelimited(&bp->pdev->dev,
> + "Timestamp not set in TX BD as expected\n");
> + return;
> + }
>
> - desc_ptp = macb_ptp_desc(queue->bp, desc);
> + desc_ptp = macb_ptp_desc(bp, desc);
> /* Unlikely but check */
> - if (!desc_ptp)
> - return -EINVAL;
> - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> - tx_timestamp = &queue->tx_timestamps[head];
> - tx_timestamp->skb = skb;
> + if (!desc_ptp) {
> + dev_warn_ratelimited(&bp->pdev->dev,
> + "Timestamp not supported in BD\n");
> + return;
> + }
> +
> /* ensure ts_1/ts_2 is loaded after ctrl (TX_USED check) */
> dma_rmb();
> - tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
> - tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
> - /* move head */
> - smp_store_release(&queue->tx_ts_head,
> - (head + 1) & (PTP_TS_BUFFER_SIZE - 1));
> -
> - schedule_work(&queue->tx_ts_task);
> - return 0;
> -}
> + gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
>
> -static void gem_tx_timestamp_flush(struct work_struct *work)
> -{
> - struct macb_queue *queue =
> - container_of(work, struct macb_queue, tx_ts_task);
> - unsigned long head, tail;
> - struct gem_tx_ts *tx_ts;
> -
> - /* take current head */
> - head = smp_load_acquire(&queue->tx_ts_head);
> - tail = queue->tx_ts_tail;
> -
> - while (CIRC_CNT(head, tail, PTP_TS_BUFFER_SIZE)) {
> - tx_ts = &queue->tx_timestamps[tail];
> - gem_tstamp_tx(queue->bp, tx_ts->skb, &tx_ts->desc_ptp);
> - /* cleanup */
> - dev_kfree_skb_any(tx_ts->skb);
> - /* remove old tail */
> - smp_store_release(&queue->tx_ts_tail,
> - (tail + 1) & (PTP_TS_BUFFER_SIZE - 1));
> - tail = queue->tx_ts_tail;
> - }
> + memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> + shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> + skb_tstamp_tx(skb, &shhwtstamps);
> }
>
> void gem_ptp_init(struct net_device *dev)
> {
> struct macb *bp = netdev_priv(dev);
> - struct macb_queue *queue;
> - unsigned int q;
>
> bp->ptp_clock_info = gem_ptp_caps_template;
>
> @@ -384,11 +344,6 @@ void gem_ptp_init(struct net_device *dev)
> }
>
> spin_lock_init(&bp->tsu_clk_lock);
> - for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> - queue->tx_ts_head = 0;
> - queue->tx_ts_tail = 0;
> - INIT_WORK(&queue->tx_ts_task, gem_tx_timestamp_flush);
> - }
>
> gem_ptp_init_tsu(bp);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: macb: simplify TX timestamp handling
2023-01-16 22:08 [PATCH net-next] net: macb: simplify TX timestamp handling Robert Hancock
2023-01-18 0:19 ` Jacob Keller
@ 2023-01-18 10:24 ` Claudiu.Beznea
2023-01-18 14:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Claudiu.Beznea @ 2023-01-18 10:24 UTC (permalink / raw)
To: robert.hancock, Nicolas.Ferre, davem, edumazet, kuba, pabeni,
richardcochran
Cc: netdev, linux-kernel
On 17.01.2023 00:08, Robert Hancock wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> This driver was capturing the TX timestamp values from the TX ring
> during the TX completion path, but deferring the actual packet TX
> timestamp updating to a workqueue. There does not seem to be much of a
> reason for this with the current state of the driver. Simplify this to
> just do the TX timestamping as part of the TX completion path, to avoid
> the need for the extra timestamp buffer and workqueue.
>
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> drivers/net/ethernet/cadence/macb.h | 29 ++-------
> drivers/net/ethernet/cadence/macb_main.c | 16 +++--
> drivers/net/ethernet/cadence/macb_ptp.c | 83 ++++++------------------
> 3 files changed, 34 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 9c410f93a103..14dfec4db8f9 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -768,8 +768,6 @@
> #define gem_readl_n(port, reg, idx) (port)->macb_reg_readl((port), GEM_##reg + idx * 4)
> #define gem_writel_n(port, reg, idx, value) (port)->macb_reg_writel((port), GEM_##reg + idx * 4, (value))
>
> -#define PTP_TS_BUFFER_SIZE 128 /* must be power of 2 */
> -
> /* Conditional GEM/MACB macros. These perform the operation to the correct
> * register dependent on whether the device is a GEM or a MACB. For registers
> * and bitfields that are common across both devices, use macb_{read,write}l
> @@ -819,11 +817,6 @@ struct macb_dma_desc_ptp {
> u32 ts_1;
> u32 ts_2;
> };
> -
> -struct gem_tx_ts {
> - struct sk_buff *skb;
> - struct macb_dma_desc_ptp desc_ptp;
> -};
> #endif
>
> /* DMA descriptor bitfields */
> @@ -1224,12 +1217,6 @@ struct macb_queue {
> void *rx_buffers;
> struct napi_struct napi_rx;
> struct queue_stats stats;
> -
> -#ifdef CONFIG_MACB_USE_HWSTAMP
> - struct work_struct tx_ts_task;
> - unsigned int tx_ts_head, tx_ts_tail;
> - struct gem_tx_ts tx_timestamps[PTP_TS_BUFFER_SIZE];
> -#endif
> };
>
> struct ethtool_rx_fs_item {
> @@ -1340,14 +1327,14 @@ enum macb_bd_control {
>
> void gem_ptp_init(struct net_device *ndev);
> void gem_ptp_remove(struct net_device *ndev);
> -int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *des);
> +void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
> void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc);
> -static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
> +static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
> {
> - if (queue->bp->tstamp_config.tx_type == TSTAMP_DISABLED)
> - return -ENOTSUPP;
> + if (bp->tstamp_config.tx_type == TSTAMP_DISABLED)
> + return;
>
> - return gem_ptp_txstamp(queue, skb, desc);
> + gem_ptp_txstamp(bp, skb, desc);
> }
>
> static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc)
> @@ -1363,11 +1350,7 @@ int gem_set_hwtst(struct net_device *dev, struct ifreq *ifr, int cmd);
> static inline void gem_ptp_init(struct net_device *ndev) { }
> static inline void gem_ptp_remove(struct net_device *ndev) { }
>
> -static inline int gem_ptp_do_txstamp(struct macb_queue *queue, struct sk_buff *skb, struct macb_dma_desc *desc)
> -{
> - return -1;
> -}
> -
> +static inline void gem_ptp_do_txstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
> static inline void gem_ptp_do_rxstamp(struct macb *bp, struct sk_buff *skb, struct macb_dma_desc *desc) { }
> #endif
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 95667b979fab..6a0419acac9d 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1191,13 +1191,9 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
> /* First, update TX stats if needed */
> if (skb) {
> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> - !ptp_one_step_sync(skb) &&
> - gem_ptp_do_txstamp(queue, skb, desc) == 0) {
> - /* skb now belongs to timestamp buffer
> - * and will be removed later
> - */
> - tx_skb->skb = NULL;
> - }
> + !ptp_one_step_sync(skb))
> + gem_ptp_do_txstamp(bp, skb, desc);
> +
> netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
> macb_tx_ring_wrap(bp, tail),
> skb->data);
> @@ -2260,6 +2256,12 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
> return ret;
> }
>
> +#ifdef CONFIG_MACB_USE_HWSTAMP
> + if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> + (bp->hw_dma_cap & HW_DMA_CAP_PTP))
> + skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> +#endif
> +
> is_lso = (skb_shinfo(skb)->gso_size != 0);
>
> if (is_lso) {
> diff --git a/drivers/net/ethernet/cadence/macb_ptp.c b/drivers/net/ethernet/cadence/macb_ptp.c
> index e6cb20aaa76a..f962a95068a0 100644
> --- a/drivers/net/ethernet/cadence/macb_ptp.c
> +++ b/drivers/net/ethernet/cadence/macb_ptp.c
> @@ -292,79 +292,39 @@ void gem_ptp_rxstamp(struct macb *bp, struct sk_buff *skb,
> }
> }
>
> -static void gem_tstamp_tx(struct macb *bp, struct sk_buff *skb,
> - struct macb_dma_desc_ptp *desc_ptp)
> +void gem_ptp_txstamp(struct macb *bp, struct sk_buff *skb,
> + struct macb_dma_desc *desc)
> {
> struct skb_shared_hwtstamps shhwtstamps;
> - struct timespec64 ts;
> -
> - gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
> - memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> - shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> - skb_tstamp_tx(skb, &shhwtstamps);
> -}
> -
> -int gem_ptp_txstamp(struct macb_queue *queue, struct sk_buff *skb,
> - struct macb_dma_desc *desc)
> -{
> - unsigned long tail = READ_ONCE(queue->tx_ts_tail);
> - unsigned long head = queue->tx_ts_head;
> struct macb_dma_desc_ptp *desc_ptp;
> - struct gem_tx_ts *tx_timestamp;
> -
> - if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl))
> - return -EINVAL;
> + struct timespec64 ts;
>
> - if (CIRC_SPACE(head, tail, PTP_TS_BUFFER_SIZE) == 0)
> - return -ENOMEM;
> + if (!GEM_BFEXT(DMA_TXVALID, desc->ctrl)) {
> + dev_warn_ratelimited(&bp->pdev->dev,
> + "Timestamp not set in TX BD as expected\n");
> + return;
> + }
>
> - desc_ptp = macb_ptp_desc(queue->bp, desc);
> + desc_ptp = macb_ptp_desc(bp, desc);
> /* Unlikely but check */
> - if (!desc_ptp)
> - return -EINVAL;
> - skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> - tx_timestamp = &queue->tx_timestamps[head];
> - tx_timestamp->skb = skb;
> + if (!desc_ptp) {
> + dev_warn_ratelimited(&bp->pdev->dev,
> + "Timestamp not supported in BD\n");
> + return;
> + }
> +
> /* ensure ts_1/ts_2 is loaded after ctrl (TX_USED check) */
> dma_rmb();
> - tx_timestamp->desc_ptp.ts_1 = desc_ptp->ts_1;
> - tx_timestamp->desc_ptp.ts_2 = desc_ptp->ts_2;
> - /* move head */
> - smp_store_release(&queue->tx_ts_head,
> - (head + 1) & (PTP_TS_BUFFER_SIZE - 1));
> -
> - schedule_work(&queue->tx_ts_task);
> - return 0;
> -}
> + gem_hw_timestamp(bp, desc_ptp->ts_1, desc_ptp->ts_2, &ts);
>
> -static void gem_tx_timestamp_flush(struct work_struct *work)
> -{
> - struct macb_queue *queue =
> - container_of(work, struct macb_queue, tx_ts_task);
> - unsigned long head, tail;
> - struct gem_tx_ts *tx_ts;
> -
> - /* take current head */
> - head = smp_load_acquire(&queue->tx_ts_head);
> - tail = queue->tx_ts_tail;
> -
> - while (CIRC_CNT(head, tail, PTP_TS_BUFFER_SIZE)) {
> - tx_ts = &queue->tx_timestamps[tail];
> - gem_tstamp_tx(queue->bp, tx_ts->skb, &tx_ts->desc_ptp);
> - /* cleanup */
> - dev_kfree_skb_any(tx_ts->skb);
> - /* remove old tail */
> - smp_store_release(&queue->tx_ts_tail,
> - (tail + 1) & (PTP_TS_BUFFER_SIZE - 1));
> - tail = queue->tx_ts_tail;
> - }
> + memset(&shhwtstamps, 0, sizeof(shhwtstamps));
> + shhwtstamps.hwtstamp = ktime_set(ts.tv_sec, ts.tv_nsec);
> + skb_tstamp_tx(skb, &shhwtstamps);
> }
>
> void gem_ptp_init(struct net_device *dev)
> {
> struct macb *bp = netdev_priv(dev);
> - struct macb_queue *queue;
> - unsigned int q;
>
> bp->ptp_clock_info = gem_ptp_caps_template;
>
> @@ -384,11 +344,6 @@ void gem_ptp_init(struct net_device *dev)
> }
>
> spin_lock_init(&bp->tsu_clk_lock);
> - for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
> - queue->tx_ts_head = 0;
> - queue->tx_ts_tail = 0;
> - INIT_WORK(&queue->tx_ts_task, gem_tx_timestamp_flush);
> - }
>
> gem_ptp_init_tsu(bp);
>
> --
> 2.39.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] net: macb: simplify TX timestamp handling
2023-01-16 22:08 [PATCH net-next] net: macb: simplify TX timestamp handling Robert Hancock
2023-01-18 0:19 ` Jacob Keller
2023-01-18 10:24 ` Claudiu.Beznea
@ 2023-01-18 14:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-01-18 14:30 UTC (permalink / raw)
To: Robert Hancock
Cc: nicolas.ferre, claudiu.beznea, davem, edumazet, kuba, pabeni,
richardcochran, netdev, linux-kernel
Hello:
This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:
On Mon, 16 Jan 2023 16:08:34 -0600 you wrote:
> This driver was capturing the TX timestamp values from the TX ring
> during the TX completion path, but deferring the actual packet TX
> timestamp updating to a workqueue. There does not seem to be much of a
> reason for this with the current state of the driver. Simplify this to
> just do the TX timestamping as part of the TX completion path, to avoid
> the need for the extra timestamp buffer and workqueue.
>
> [...]
Here is the summary with links:
- [net-next] net: macb: simplify TX timestamp handling
https://git.kernel.org/netdev/net-next/c/8e7610e686d0
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-18 14:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-16 22:08 [PATCH net-next] net: macb: simplify TX timestamp handling Robert Hancock
2023-01-18 0:19 ` Jacob Keller
2023-01-18 10:24 ` Claudiu.Beznea
2023-01-18 14:30 ` patchwork-bot+netdevbpf
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.