Re: [PATCH net-next v3 1/4] net: wangxun: Add support for PTP clock

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 13/01/2025 07:16, Jiawen Wu wrote:
>>> @@ -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.

Ok, that's what I actually expected. Could you please use seqlock_t
instead of plain spinlock - there is a clear split of readers and
writers for cycle counter.