> -----Original Message-----
> From: Kitszel, Przemyslaw <przemyslaw.kits...@intel.com>
> Sent: Friday, July 14, 2023 4:39 AM
> To: linuxptp-devel@lists.sourceforge.net
> Cc: Richard Cochran <richardcoch...@gmail.com>; Miroslav Lichvar
> <mlich...@redhat.com>; Keller, Jacob E <jacob.e.kel...@intel.com>; Kolacinski,
> Karol <karol.kolacin...@intel.com>; Glaza, Jan <jan.gl...@intel.com>; Plachno,
> Lukasz <lukasz.plac...@intel.com>; Pacuszka, MateuszX
> <mateuszx.pacus...@intel.com>; Czapnik, Lukasz <lukasz.czap...@intel.com>;
> Kitszel, Przemyslaw <przemyslaw.kits...@intel.com>
> Subject: [PATCH] sk: don't report random errno on timeout
>
> Fix errno value reported up from sk_receive() on poll() timeout.
>
> With neither caller nor poll() itself zeroing errno value, it will contain
> result of previous failure, possibly from long time ago.
>
> Reporting errno=0 up from sk_receive() would bring confusion,
> as "%m" is later used in pr_err() (so one would get "error Success").
> Use ETIME as it fits here the best.
> (ETIMEDOUT instead of ETIME would look better in the code,
> but message printed would be worse).
>
> Prior to this patch, following log could be produced:
> | timed out while polling for tx timestamp
> | increasing tx_timestamp_timeout may correct this issue, but it is likely
> caused
> by a driver bug
> | PTP send sync failed : error No such device or address
>
> With this patch applied, one will get proper error in last line,
> "Timer expired", and more modern suggestion about how to approach fixing it
>
I think changing the message about what might be causing timeout is
unnecessary. It may be helpful purely in the context of some devices, but it is
not a good general message as not all hardware and drivers have the same
design. In the *general* case if this timeout is hit then it is usually a bug
in the driver for that hardware. In the specific case for ice hardware, the
mention of thread starvation is accurate, but that is unlikely to be general
across all hardware.
Thus, I think we should leave the error message alone and just fix the errno
value. Improving the errno value is important since it would be less confusing
than seeing arbitrary error values which are unrelated to the actual error.
Thanks,
Jake
>
> Signed-off-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
> ---
> sk.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/sk.c b/sk.c
> index a72aca3b7821..d95002b24cc6 100644
> --- a/sk.c
> +++ b/sk.c
> @@ -441,12 +441,15 @@ int sk_receive(int fd, void *buf, int buflen,
> /* Retry once on EINTR to avoid logging errors before exit */
> if (res < 0 && errno == EINTR)
> res = poll(&pfd, 1, sk_tx_timeout);
> - if (res < 1) {
> - pr_err(res ? "poll for tx timestamp failed: %m" :
> - "timed out while polling for tx
> timestamp");
> - pr_err("increasing tx_timestamp_timeout may correct "
> - "this issue, but it is likely caused by a driver
> bug");
> + if (res < 0) {
> + pr_err("poll for tx timestamp failed: %m");
> return -errno;
> + } else if (!res) {
> + pr_err("timed out while polling for tx timestamp");
> + pr_err("increasing tx_timestamp_timeout may correct
> this issue,"
> + "but it is likely caused by starving some of
> PTP/driver
> threads");
> + errno = ETIME;
> + return -1;
> } else if (!(pfd.revents & sk_revents)) {
> pr_err("poll for tx timestamp woke up on non ERR
> event");
> return -1;
>
> base-commit: aa60db270be12e7144e7b0cef2f5dde4213b7057
> --
> 2.38.1
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
