[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <057c01db658b$1e6f45f0$5b4dd1d0$@trustnetic.com>
Date: Mon, 13 Jan 2025 15:16:51 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Vadim Fedorenko'" <vadim.fedorenko@...ux.dev>,
<andrew+netdev@...n.ch>,
<davem@...emloft.net>,
<edumazet@...gle.com>,
<kuba@...nel.org>,
<pabeni@...hat.com>,
<richardcochran@...il.com>,
<linux@...linux.org.uk>,
<horms@...nel.org>,
<jacob.e.keller@...el.com>,
<netdev@...r.kernel.org>
Cc: <mengyuanlou@...-swift.com>
Subject: RE: [PATCH net-next v3 1/4] net: wangxun: Add support for PTP clock
> > @@ -1501,12 +1535,19 @@ static netdev_tx_t wx_xmit_frame_ring(struct sk_buff *skb,
> > if (test_bit(WX_FLAG_FDIR_CAPABLE, wx->flags) && tx_ring->atr_sample_rate)
> > wx->atr(tx_ring, first, ptype);
> >
> > - wx_tx_map(tx_ring, first, hdr_len);
> > + if (wx_tx_map(tx_ring, first, hdr_len))
> > + goto cleanup_tx_tstamp;
> >
> > return NETDEV_TX_OK;
> > out_drop:
> > dev_kfree_skb_any(first->skb);
> > first->skb = NULL;
> > +cleanup_tx_tstamp:
> > + if (unlikely(tx_flags & WX_TX_FLAGS_TSTAMP)) {
> > + dev_kfree_skb_any(wx->ptp_tx_skb);
> > + wx->ptp_tx_skb = NULL;
> > + clear_bit_unlock(WX_STATE_PTP_TX_IN_PROGRESS, wx->state);
> > + }
>
> This is error path of dma mapping, means TX timestamp will be missing
> because the packet was not sent. But the error/missing counter is not
> bumped. I think it needs to be indicated.
I'll count it as 'err' in ethtool_ts_stats.
> > +static int wx_ptp_set_timestamp_mode(struct wx *wx,
> > + struct kernel_hwtstamp_config *config)
> > +{
> > + u32 tsync_tx_ctl = WX_TSC_1588_CTL_ENABLED;
> > + u32 tsync_rx_ctl = WX_PSR_1588_CTL_ENABLED;
> > + DECLARE_BITMAP(flags, WX_PF_FLAGS_NBITS);
> > + u32 tsync_rx_mtrl = PTP_EV_PORT << 16;
> > + bool is_l2 = false;
> > + u32 regval;
> > +
> > + memcpy(flags, wx->flags, sizeof(wx->flags));
> > +
> > + switch (config->tx_type) {
> > + case HWTSTAMP_TX_OFF:
> > + tsync_tx_ctl = 0;
> > + break;
> > + case HWTSTAMP_TX_ON:
> > + break;
> > + default:
> > + return -ERANGE;
> > + }
> > +
> > + switch (config->rx_filter) {
> > + case HWTSTAMP_FILTER_NONE:
> > + tsync_rx_ctl = 0;
> > + tsync_rx_mtrl = 0;
> > + clear_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > + clear_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > + break;
> > + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC:
> > + tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
> > + tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_SYNC;
> > + set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > + set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > + break;
> > + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ:
> > + tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_L4_V1;
> > + tsync_rx_mtrl |= WX_PSR_1588_MSG_V1_DELAY_REQ;
> > + set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > + set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > + break;
> > + case HWTSTAMP_FILTER_PTP_V2_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT:
> > + case HWTSTAMP_FILTER_PTP_V2_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC:
> > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ:
> > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ:
> > + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ:
> > + tsync_rx_ctl |= WX_PSR_1588_CTL_TYPE_EVENT_V2;
> > + is_l2 = true;
> > + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT;
> > + set_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, flags);
> > + set_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, flags);
> > + break;
> > + default:
> > + /* register RXMTRL must be set in order to do V1 packets,
> > + * therefore it is not possible to time stamp both V1 Sync and
> > + * Delay_Req messages unless hardware supports timestamping all
> > + * packets => return error
> > + */
> > + clear_bit(WX_FLAG_RX_HWTSTAMP_ENABLED, wx->flags);
> > + clear_bit(WX_FLAG_RX_HWTSTAMP_IN_REGISTER, wx->flags);
> > + config->rx_filter = HWTSTAMP_FILTER_NONE;
> > + return -ERANGE;
>
> looks like this code is a bit tricky and leads to out-of-sync
> configuration. Imagine the situation when HWTSTAMP_FILTER_PTP_V2_EVENT
> was configured first, the hardware was properly set up and timestamps
> are coming. wx->flags will have bits WX_FLAG_RX_HWTSTAMP_ENABLED and
> WX_FLAG_RX_HWTSTAMP_IN_REGISTER set. Then the user asks to enable
> HWTSTAMP_FILTER_ALL, which is not supported. wx->flags will have bits
> mentioned above cleared, but the hardware will still continue to
> timestamp some packets.
You are right. I'll remove the bit clears in the default case.
> > +void wx_ptp_reset(struct wx *wx)
> > +{
> > + unsigned long flags;
> > +
> > + /* reset the hardware timestamping mode */
> > + wx_ptp_set_timestamp_mode(wx, &wx->tstamp_config);
> > + wx_ptp_reset_cyclecounter(wx);
> > +
> > + wr32ptp(wx, WX_TSC_1588_SYSTIML, 0);
> > + wr32ptp(wx, WX_TSC_1588_SYSTIMH, 0);
> > + WX_WRITE_FLUSH(wx);
>
> writes to WX_TSC_1588_SYSTIML/WX_TSC_1588_SYSTIMH are not protected by
> tmreg_lock, while reads are protected in wx_ptp_read() and in
> wx_ptp_gettimex64()
No need to protect it. See below.
> > @@ -1133,6 +1168,21 @@ struct wx {
> > void (*atr)(struct wx_ring *ring, struct wx_tx_buffer *first, u8 ptype);
> > void (*configure_fdir)(struct wx *wx);
> > void (*do_reset)(struct net_device *netdev);
> > +
> > + u32 base_incval;
> > + u32 tx_hwtstamp_pkts;
> > + u32 tx_hwtstamp_timeouts;
> > + u32 tx_hwtstamp_skipped;
> > + u32 rx_hwtstamp_cleared;
> > + unsigned long ptp_tx_start;
> > + spinlock_t tmreg_lock; /* spinlock for ptp */
>
> Could you please explain what this lock protects exactly? According to
> the name, it should serialize access to tm(?) registers, but there is
> a mix of locked and unlocked accesses in the code ...
> If this lock protects cyclecounter/timecounter then it might be better
> to use another name, like hw_cc_lock. And in this case it's even better
> to use seqlock_t with reader/writer accessors according to the code path.
It is for struct timecounter. The registers are read only to update the cycle
counter. I think it's better to name it hw_tc_lock.
Powered by blists - more mailing lists