On Thu, 8 Apr 2021 19:13:50 +0800 Yangbo Lu wrote:
> This patch is to add support for PTP Sync packet one-step timestamping.
> Since ENETC single-step register has to be configured dynamically per
> packet for correctionField offeset and UDP checksum update, current
> one-step timestamping packet has to be sent only when the last one
> completes transmitting on hardware. So, on the TX below things are done
> by the patch:
>
> - For one-step timestamping packet, queue to skb queue.
> - Start a work to transmit skbs in queue.
> - For other skbs, transmit immediately.
> - mutex lock used to ensure the last one-step timestamping packet has
> already been transmitted on hardware before transmitting current one.
>
> And the configuration for one-step timestamping on ENETC before
> transmitting is,
>
> - Set one-step timestamping flag in extension BD.
> - Write 30 bits current timestamp in tstamp field of extension BD.
> - Update PTP Sync packet originTimestamp field with current timestamp.
> - Configure single-step register for correctionField offeset and UDP
> checksum update.
>
> Signed-off-by: Yangbo Lu <yangbo...@nxp.com>
> @@ -432,9 +544,12 @@ static bool enetc_clean_tx_ring(struct enetc_bdr
> *tx_ring, int napi_budget)
> xdp_return_frame(xdp_frame);
> tx_swbd->xdp_frame = NULL;
> } else if (skb) {
> - if (unlikely(do_tstamp)) {
> + if (unlikely(tx_swbd->skb->cb[0] &
> + ENETC_F_TX_ONESTEP_SYNC_TSTAMP)) {
> + mutex_unlock(&priv->onestep_tstamp_lock);
> + } else if (unlikely(do_twostep_tstamp)) {
> enetc_tstamp_tx(skb, tstamp);
> - do_tstamp = false;
> + do_twostep_tstamp = false;
> }
> napi_consume_skb(skb, napi_budget);
> tx_swbd->skb = NULL;
> @@ -1863,6 +1978,47 @@ static int enetc_phylink_connect(struct net_device
> *ndev)
> return 0;
> }
>
> +static void enetc_tx_onestep_tstamp(struct work_struct *work)
> +{
> + struct enetc_ndev_priv *priv;
> + struct sk_buff *skb;
> +
> + priv = container_of(work, struct enetc_ndev_priv, tx_onestep_tstamp);
> +
> + while (true) {
> + skb = skb_dequeue(&priv->tx_skbs);
> + if (!skb)
> + return;
> +
> + /* Lock before TX one-step timestamping packet, and release
> + * when the packet has been sent on hardware, or transmit
> + * failure.
> + */
> + mutex_lock(&priv->onestep_tstamp_lock);
Using a lock to wake up a producer is not a great idea. It usually
breaks advanced features like priority inheritance. Probably doesn't
matter for a struct mutex, but I think it may still make lockdep
complain.
Why not make it work with a flag?
start_xmit:
if (skb->cb[0] & ONESTEP) {
if (priv->flags & ONESTEP_BUSY) {
skb_queue_tail(&priv->tx_skbs, skb);
return ...;
}
priv->flags |= ONESTEP_BUSY;
}
clean_tx:
/* don't clear ONESTEP_BUSY, we need the tx lock */
if (skb->cb[0] & ONESTEP)
queue_work(...);
work:
netif_tx_lock()
skb = skb_dequeue();
if (skb)
start_xmit(skb)
else
priv->flags &= ~ONESTEP_BUSY;
netif_tx_unlock()
> + netif_tx_lock(priv->ndev);
> + enetc_start_xmit(skb, priv->ndev);
> + netif_tx_unlock(priv->ndev);
> + }
> +}
> +
> +static int enetc_tx_onestep_tstamp_init(struct enetc_ndev_priv *priv)
> +{
> + priv->enetc_ptp_wq = alloc_workqueue("enetc_ptp_wq", 0, 0);
> + if (!priv->enetc_ptp_wq)
> + return -ENOMEM;
> +
> + INIT_WORK(&priv->tx_onestep_tstamp, enetc_tx_onestep_tstamp);
> + skb_queue_head_init(&priv->tx_skbs);
> +
> + return 0;
> +}
> +
> +static void enetc_tx_onestep_tstamp_deinit(struct enetc_ndev_priv *priv)
> +{
> + destroy_workqueue(priv->enetc_ptp_wq);
> +}
Why allocate a separate workqueue for one work? You can queue your
work on the system workqueue.
