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.
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!
Regards,
Claudiu
