Re: [PATCH iwl-net v3] ice/ptp: fix crosstimestamp reporting

[Simon Horman <horms@kernel.org>]

On Thu, May 15, 2025 at 02:32:36PM +0200, Anton Nadezhdin wrote:
> Set use_nsecs=true as timestamp is reported in ns. Lack of this result
> in smaller timestamp error window which cause error during phc2sys
> execution on E825 NICs:
> phc2sys[1768.256]: ioctl PTP_SYS_OFFSET_PRECISE: Invalid argument
> 
> Before commit "Provide infrastructure for converting to/from a base clock"

Hi Anton,

Thanks for your patch.

I have some feedback on the form of the commit message, I hope it is useful.

The correct syntax for fully citing a commit is:
commit 6b2e29977518 ("timekeeping: Provide infrastructure for converting
to/from a base clock")

> the parameter use_nsec was not required. "Remove convert_art_to_tsc()"
> rework shall already contain use_nsecs=true.

I think it would be clearer to express this something along these lines.

  This problem was introduced in commit d4bea547ebb57 ("ice/ptp: Remove
  convert_art_to_tsc()") which omitted setting use_nsecs to true when
  converting the ice driver to use convert_base_to_cs().

Or if there is only one Fixes tag, as I propose below, and there is
no reference to any other commit in the commit message, you could shorten
it a bit to.

  This problem was introduced in the cited commit which omitted setting
  use_nsecs to true when converting the ice driver to use
  convert_base_to_cs().

OTOH, if you think it is useful you could add something like this. But
IMHO it isn't adding much value and I'd leave it out based on less being
more.

  convert_base_to_cs() was added by commit 6b2e29977518 ("timekeeping:
  Provide infrastructure for converting to/from a base clock").

> 
> Testing hints (ethX is PF netdev):
> phc2sys -s ethX -c CLOCK_REALTIME  -O 37 -m
> phc2sys[1769.256]: CLOCK_REALTIME phc offset -5 s0 freq      -0 delay    0
> 
> Fixes: d4bea547ebb57 ("ice/ptp: Remove convert_art_to_tsc()")

My understanding is that the patch cited above (d4bea547ebb57) introduced
the bug being addressed. And that it did so by incorrectly using the
infrastructure added by the patch cited below (6b2e29977518e).

Thus, as the aim is to highlight which commit introduced the bug, I think
that only the fixes tag above should be present (and the one below should
be removed).

> Fixes" 6b2e29977518e ("timekeeping: Provide infrastructure for converting to/from a base clock")

Not strictly relevant if this tag is removed (see above)
but the correct syntax is Fixes: ... (not Fixes" ...).

> Signed-off-by: Anton Nadezhdin <anton.nadezhdin@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>

The code change itself looks correct to me.

...