RE: [PATCH net-next v1 1/6] ethtool: add interface to read Tx hardware timestamping statistics

From: Keller, Jacob E
Date: Wed Apr 03 2024 - 14:45:26 EST




> -----Original Message-----
> From: Rahul Rameshbabu <rrameshbabu@xxxxxxxxxx>
> Sent: Tuesday, April 2, 2024 10:15 PM
> To: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; Zaki, Ahmed <ahmed.zaki@xxxxxxxxx>; Lobakin, Aleksander
> <aleksander.lobakin@xxxxxxxxx>; alexandre.torgue@xxxxxxxxxxx;
> andrew@xxxxxxx; cjubran@xxxxxxxxxx; corbet@xxxxxxx; davem@xxxxxxxxxxxxx;
> dtatulea@xxxxxxxxxx; edumazet@xxxxxxxxxx; gal@xxxxxxxxxx;
> hkallweit1@xxxxxxxxx; Keller, Jacob E <jacob.e.keller@xxxxxxxxx>;
> jiri@xxxxxxxxxxx; joabreu@xxxxxxxxxxxx; justinstitt@xxxxxxxxxx;
> kory.maincent@xxxxxxxxxxx; leon@xxxxxxxxxx; liuhangbin@xxxxxxxxx;
> maxime.chevallier@xxxxxxxxxxx; pabeni@xxxxxxxxxx; Greenwalt, Paul
> <paul.greenwalt@xxxxxxxxx>; Kitszel, Przemyslaw <przemyslaw.kitszel@intelcom>;
> rdunlap@xxxxxxxxxxxxx; richardcochran@xxxxxxxxx; saeed@xxxxxxxxxx;
> tariqt@xxxxxxxxxx; vadim.fedorenko@xxxxxxxxx; vladimir.oltean@xxxxxxx;
> Drewek, Wojciech <wojciech.drewek@xxxxxxxxx>
> Subject: Re: [PATCH net-next v1 1/6] ethtool: add interface to read Tx hardware
> timestamping statistics
>
> On Tue, 02 Apr, 2024 19:18:42 -0700 Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > On Tue, 2 Apr 2024 13:52:01 -0700 Rahul Rameshbabu wrote:
> >> +/**
> >> + * struct ethtool_ts_stats - HW timestamping statistics
> >> + * @tx_stats: struct group for TX HW timestamping
> >> + * @pkts: Number of packets successfully timestamped by the hardware.
> >> + * @lost: Number of hardware timestamping requests where the
> timestamping
> >> + * information from the hardware never arrived for submission with
> >> + * the skb.
> >> + * @err: Number of arbitrary timestamp generation error events that the
> >> + * hardware encountered, exclusive of @lost statistics. Cases such
> >> + * as resource exhaustion, unavailability, firmware errors, and
> >> + * detected illogical timestamp values not submitted with the skb
> >> + * are inclusive to this counter.
> >> + */
> >> +struct ethtool_ts_stats {
> >> + struct_group(tx_stats,
> >
> > Doesn't seem like the group should be documented:
> >
> > include/linux/ethtool.h:503: warning: Excess struct member 'tx_stats'
> description in 'ethtool_ts_stats'
>
> Was looking into why our internal verification did not catch this. We
> run W=1 with clang, but looks like the warning does not get triggered
> unless explicitly run with scripts/kernel-doc.
>
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-
> format-kernel-doc-comments
>
> I have debugged using strace that the way the kernel doc checking works
> when W=1 is set is that the matching source file that is being compiled
> is passed to scripts/kernel-doc, so include files are missed from the
> doc check. I think this is worth adding to the kernel documentation.
>

It would be great if the W=1 setup could figure out the include files and send those to kernel-doc too, but I'm not sure if this is possible and if so how difficult it would be to implement it. A lot of headers produce warnings because a lot fewer people manually run kernel-doc on the entire source.

> Anyway, I will send out a v2 with this fixed but wait for potentially
> more feedback on v1.
>
> --
> Thanks,
>
> Rahul Rameshbabu