Hi Claudiu,
> -----Original Message-----
> From: Claudiu Manoil <claudiu.man...@gmail.com>
> Sent: 2021年3月28日 15:52
> To: Y.b. Lu <yangbo...@nxp.com>; netdev@vger.kernel.org
> Cc: David S . Miller <da...@davemloft.net>; Richard Cochran
> <richardcoch...@gmail.com>; Claudiu Manoil <claudiu.man...@nxp.com>;
> Jakub Kicinski <k...@kernel.org>; Vladimir Oltean <vladimir.olt...@nxp.com>
> Subject: Re: [PATCH 2/2] enetc: support PTP Sync packet one-step
> timestamping
>
> Hi Yangbo,
> Pls add the [net-next] prefix to the subject of these patches next time, to
> avoid
> the patchwork warnings and let reviewers know where to apply them.
Thanks. I added that for v2.
>
> On 26.03.2021 10:35, Yangbo Lu wrote:
> [...]> +netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
> > +{
> > + struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > + u8 udp, msgtype, twostep;
> > + u16 offset1, offset2;
> > +
> > + /* Mark tx timestamp type on skb->cb[0] if requires */
> > + if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
> > + (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK)) {
> > + skb->cb[0] = priv->active_offloads & ENETC_F_TX_TSTAMP_MASK;
> > + } else {
> > + skb->cb[0] = 0;
> > + }
> > +
> > + if (skb->cb[0] & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> > + /* For one-step PTP sync packet, queue it */
> > + if (!enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
> > + &offset1, &offset2)) {
> > + if (msgtype == PTP_MSGTYPE_SYNC && twostep == 0) {
> > + skb_queue_tail(&priv->tx_skbs, skb);
> > + queue_work(priv->enetc_ptp_wq,
> > + &priv->tx_onestep_tstamp);
> > + return NETDEV_TX_OK;
> > + }
> > + }
> > +
> > + /* Fall back to two-step timestamp for other packets */
> > + skb->cb[0] = ENETC_F_TX_TSTAMP;
> > + }
> > +
> > + return enetc_start_xmit(skb, ndev);
> > +}
> > +
> [...]
> > +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);
> > + enetc_start_xmit(skb, priv->ndev);
> > + }
> > +}
> > +
> What happens if the work queue tries to send the ptp packet concurrently with
> a regular packet being sent by the stack, via .ndo_start_xmit?
> If both skbs are targetting the same tx_ring then we have a concurrency
> problem, as enetc_map_tx_buffs(tx_ring, skb) is not thread safe!
Thanks a lot for pointing out this problem.
I tried to use netif_tx_lock for one-step timestamping packet sending in v2,
per your kind suggestion offline.
>
> Regards,
> Claudiu
