netdev - Re: [PATCH net] 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]

Message-ID: <aa6edec3-4be8-d1e4-159f-1659aa4e0bbe@linux.dev>
Date:   Tue, 7 Mar 2023 10:25:04 +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] bnxt_en: reset PHC frequency in free-running mode

On 07/03/2023 05:07, Pavan Chebbi wrote:
> On Tue, Mar 7, 2023 at 12:43 AM Vadim Fedorenko
> <vadim.fedorenko@...ux.dev> wrote:
>>
>> On 06/03/2023 17:11, Pavan Chebbi wrote:
>>> On Mon, Mar 6, 2023 at 10:23 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_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)
>>>> +                       bnxt_ptp_adjfreq_rtc(bp, 0);
> 
> I am not sure if the intended objective of resetting the PHC is going
> to be achieved with this. The FW will always apply the new ppb on the
> base PHC frequency. I am not sure what you mean by "reset the hardware
> frequency of PHC to zero" in the commit message.

Well, I meant reset it to base frequency and remove any adjustments 
applied before. I'll re-phrase it in the next version.

> If you want PHC to
> start counting from 0 on init, you may use bnxt_ptp_cfg_settime() with
> 0.

That's not what we want, the current counter behavior is ok.

> Also, the RTC flag may not be set on newer firmwares running on MH
> systems. I see no harm in resetting the PHC unconditionally if we are
> not in RTC mode.

And that's perfectly fine! Each firmware upgrade requires full NIC reset 
and it will end up with reset of PHC to base frequency. But in the 
situation when we have several version of kernel running on the SLED, on 
kernel upgrade we end up with adjustment stored in the PHC using old 
kernel which was tuning RTC, but then booted into new kernel which stops 
tuning RTC. If the last stored adjustment was close to boundaries for 
some reasons, timecounter will never compensate the difference and all 
hosts in the sled will be drifting hard. The only way to bring it back 
to working state is to power reset the sled, which is disruptive action.

The fix was proved to work actually in our setup.


>>>
>>> You meant bnxt_ptp_adjfine_rtc(), right.
>>> Anyway, let me go through the patch in detail, while you may submit
>>> corrections for the build.
>>>
>> Oh, yeah, right, artefact of rebasing. Will fix it in v2, thanks.
>>
>>
>>>>           }
>>>>
>>>>           ptp->ptp_info = bnxt_ptp_caps;
>>>> --
>>>> 2.30.2
>>>>
>>