| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d41bdce0-7ae8-47cf-a713-7305c7b7a8b7@linux.dev>
Date: Mon, 13 Jan 2025 10:38:13 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Jiawen Wu <jiawenwu@...stnetic.com>, 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
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.
Powered by blists - more mailing lists