netdev - Re: [PATCH net v2] bnxt_en: reset PHC frequency in free-running mode

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]

Date:   Thu, 9 Mar 2023 17:53:02 +0530
From:   Pavan Chebbi <pavan.chebbi@...adcom.com>
To:     Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc:     Vadim Fedorenko <vadfed@...a.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Andy Gospodarek <andrew.gospodarek@...adcom.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Richard Cochran <richardcochran@...il.com>,
        netdev@...r.kernel.org
Subject: Re: [PATCH net v2] bnxt_en: reset PHC frequency in free-running mode

On Thu, Mar 9, 2023 at 4:20 PM Vadim Fedorenko
<vadim.fedorenko@...ux.dev> wrote:
>
> On 09/03/2023 10:11, Pavan Chebbi wrote:
> > On Thu, Mar 9, 2023 at 3:02 PM Vadim Fedorenko
> > <vadim.fedorenko@...ux.dev> wrote:
> >>
> >> On 09.03.2023 04:40, Pavan Chebbi wrote:
> >>> On Wed, Mar 8, 2023 at 8:12 PM Vadim Fedorenko <vadfed@...a.com> wrote:
> >
> >>>> @@ -932,13 +937,15 @@ int bnxt_ptp_init(struct bnxt *bp, bool phc_cfg)
> >>>>           atomic_set(&ptp->tx_avail, BNXT_MAX_TX_TS);
> >>>>           spin_lock_init(&ptp->ptp_lock);
> >>>>
> >>>> -       if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC) {
> >>>> +       if (BNXT_PTP_USE_RTC(ptp->bp)) {
> >>>>                   bnxt_ptp_timecounter_init(bp, false);
> >>>>                   rc = bnxt_ptp_init_rtc(bp, phc_cfg);
> >>>>                   if (rc)
> >>>>                           goto out;
> >>>>           } else {
> >>>>                   bnxt_ptp_timecounter_init(bp, true);
> >>>> +               if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
> >>>
> >>> I understand from your response on v1 as to why it will not affect you
> >>> if a new firmware does not report RTC on MH.
> >>> However, once you update the fw, any subsequent kernels upgrades will
> >>> prevent resetting the freq stored in the PHC.
> >>> Would changing the check to if (BNXT_MH(bp)) instead be a better option?
> >>
> >> How will it affect hardware without RTC support? The one which doesn't have
> >> BNXT_FW_CAP_PTP_RTC in a single-host configuration. Asking because if FW will
> >
> > For single hosts, it should not matter if we reset the PHC frequency.
> > bnxt_ptp_init() is [re]initializing the host-timecounter, and this
> > function being called on a single host means everything is going to
> > [re]start from scratch.
> >
> >> not expose BNXT_FW_CAP_PTP_RTC, the check BNXT_PTP_USE_RTC() will be equal to
> >> !BNXT_MH() and there will be no need for additional check in this else clause.
> >
> > You are right, hence my original suggestion of resetting the PHC freq
> > unconditionally is better.
> > One more thing, the function bnxt_ptp_adjfine() should select
> > non-realtime adjustments only for MH systems.
> > You may need to flip the check, something like
> >
> > if (!BNXT_MH(ptp->bp))
> >      return bnxt_ptp_adjfine_rtc(bp, scaled_ppm);
> >
> > This is because there can be a very old firmware which does not have
> > RTC on single hosts but we still want to make HW adjustments.
>
> Well, I just want to be sure that we will support all possible
> combinations of FW in the driver. AFAIU, there 3 different options:
>
> 1. Very old firmware. Doesn't have RTC support and doesn't expose
> BNXT_FW_CAP_PTP_RTC. Call to bnxt_ptp_adjfine_rtc on this variant may
> make HW unusable. We MUST not call it in this case. The timecounter is
> also not supported in this configuration, right?

No, the first ever version of bnxt PTP solution has the FW support for
HW adjustment but does not expose BNXT_FW_CAP_PTP_RTC.
So we need to be mindful of this just to keep backward compatibility intact.

> 2. Firmware which supports RTC and exposes BNXT_FW_CAP_PTP_RTC.
> Timecounter should be used only in MH case then.

Timecounter is used in all variations. Single host (SH) and Multi host
(MH), with/without RTC. The only difference is how timecounter is
used.
With RTC, we wanted to provide an MH solution to achieve
synchronization across all the hosts using a common timebase.
But we had to implement the recently added non-realtime design to
achieve that goal. But we still need RTC on single hosts when we want
to adjust phase.

> 3. Firmware which supports RTC, but doesn't expose BNXT_FW_CAP_PTP_RTC
> for MH configuration. How can we understand that it's not variant 1 in
> MH configuration? Or are we sure FUNC_QCAPS_RESP_FLAGS_PTP_SUPPORTED is
> not set on old firmware?

As such we don't use RTC (even if exposed) at all in MH case. So
simply ignore the RTC if you determine a MH system.

Let me just summarize:
we can have three variants of Firmware.
1. That does not have RTC exposed on both SH and MH (very old)
2. That has RTC exposed both on SH and MH (current production)
3. That has RTC exposed on SH and not on MH (future)

In all the above cases, we make use of the timecounter for timestamps,
but we must make HW freq adjustments in SH, and timecounter freq
adjustments in MH.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)