netdev - RE: [PATCH net-next 08/10] idpf: add Tx timestamp flows

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <MW4PR11MB588902CFD752740F584854798EAF2@MW4PR11MB5889.namprd11.prod.outlook.com>
Date: Wed, 2 Apr 2025 14:57:30 +0000
From: "Olech, Milena" <milena.olech@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "pabeni@...hat.com"
	<pabeni@...hat.com>, "Dumazet, Eric" <edumazet@...gle.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "Kolacinski, Karol"
	<karol.kolacinski@...el.com>, "richardcochran@...il.com"
	<richardcochran@...il.com>, "Hay, Joshua A" <joshua.a.hay@...el.com>, "Salin,
 Samuel" <samuel.salin@...el.com>
Subject: RE: [PATCH net-next 08/10] idpf: add Tx timestamp flows

On 03/25/2025 2:01 PM, Jakub Kicinski wrote:

>On Tue, 18 Mar 2025 09:13:23 -0700 Tony Nguyen wrote:
>> +/**
>> + * struct idpf_tx_tstamp_stats - Tx timestamp statistics
>> + * @tx_hwtstamp_lock: Lock to protect Tx tstamp stats
>> + * @tx_hwtstamp_discarded: Number of Tx skbs discarded due to cached PHC time
>> + *			   being too old to correctly extend timestamp
>> + * @tx_hwtstamp_flushed: Number of Tx skbs flushed due to interface closed
>> + */
>> +struct idpf_tx_tstamp_stats {
>> +	struct mutex tx_hwtstamp_lock;
>> +	u32 tx_hwtstamp_discarded;
>> +	u32 tx_hwtstamp_flushed;
>> +};
>
>>   * idpf_get_rxnfc - command to get RX flow classification rules
>> @@ -479,6 +480,9 @@ static const struct idpf_stats idpf_gstrings_port_stats[] = {
>>  	IDPF_PORT_STAT("tx-unicast_pkts", port_stats.vport_stats.tx_unicast),
>>  	IDPF_PORT_STAT("tx-multicast_pkts", port_stats.vport_stats.tx_multicast),
>>  	IDPF_PORT_STAT("tx-broadcast_pkts", port_stats.vport_stats.tx_broadcast),
>> +	IDPF_PORT_STAT("tx-hwtstamp_skipped", port_stats.tx_hwtstamp_skipped),
>> +	IDPF_PORT_STAT("tx-hwtstamp_flushed", tstamp_stats.tx_hwtstamp_flushed),
>> +	IDPF_PORT_STAT("tx-hwtstamp_discarded", tstamp_stats.tx_hwtstamp_discarded),
>
>I don't see you implementing .get_ts_stats ? If there is a reason
>please explain in the commit msg. We require that standard stats
>are reported if you want to report custom, more granular ones.

TBH I wasn't aware of that :<

Do you think that the lack of this stats is a blocker for this series?

>
>> +static int idpf_get_ts_info(struct net_device *netdev,
>> +			    struct kernel_ethtool_ts_info *info)
>> +{
>> +	struct idpf_netdev_priv *np = netdev_priv(netdev);
>> +	struct idpf_vport *vport;
>> +	int err = 0;
>> +
>> +	if (!mutex_trylock(&np->adapter->vport_ctrl_lock))
>
>Why trylock? This also needs a solid and well documented justification
>to pass.

Right - in most places idpf_vport_ctrl_lock is used, but I guess that for
PTP use-case is safer to return an error immediately when the vport cannot
be accessed - for example because of reset.

Thanks,
Milena

>-- 
>pw-bot: cr
>