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] [day] [month] [year] [list]
Date:   Thu, 9 Mar 2023 15:37:44 +0000
From:   Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To:     Pavan Chebbi <pavan.chebbi@...adcom.com>
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 09/03/2023 12:23, Pavan Chebbi wrote:
> 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.

OK, got it. I'll send a v3 soon with the comments addressed.
Thanks for feedback and explanations.