| 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
| ||
|
Message-ID: <d8a49972-2875-2be9-a210-92d9dac32c03@linux.dev>
Date: Mon, 6 Mar 2023 19:12:59 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Pavan Chebbi <pavan.chebbi@...adcom.com>,
Vadim Fedorenko <vadfed@...a.com>
Cc: 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 06/03/2023 17:11, Pavan Chebbi wrote:
> On Mon, Mar 6, 2023 at 10:23 PM Vadim Fedorenko <vadfed@...a.com> wrote:
>>
>> When using a PHC in shared between multiple hosts, the previous
>> frequency value may not be reset and could lead to host being unable to
>> compensate the offset with timecounter adjustments. To avoid such state
>> reset the hardware frequency of PHC to zero on init. Some refactoring is
>> needed to make code readable.
>>
> Thanks for the patch.
> I see what you are trying to do. But I think we have some build issues
> with this.
> Haven't looked at the whole patch, but one error I can spot is down at
> the bottom.
>
>> Fixes: 85036aee1938 ("bnxt_en: Add a non-real time mode to access NIC clock")
>> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
>> ---
>> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 6 +-
>> drivers/net/ethernet/broadcom/bnxt/bnxt.h | 2 +
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 57 +++++++++++--------
>> 3 files changed, 36 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index 5d4b1f2ebeac..8472ff79adf3 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -6989,11 +6989,9 @@ static int bnxt_hwrm_func_qcfg(struct bnxt *bp)
>> if (flags & FUNC_QCFG_RESP_FLAGS_FW_DCBX_AGENT_ENABLED)
>> bp->fw_cap |= BNXT_FW_CAP_DCBX_AGENT;
>> }
>> - if (BNXT_PF(bp) && (flags & FUNC_QCFG_RESP_FLAGS_MULTI_HOST)) {
>> + if (BNXT_PF(bp) && (flags & FUNC_QCFG_RESP_FLAGS_MULTI_HOST))
>> bp->flags |= BNXT_FLAG_MULTI_HOST;
>> - if (bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>> - bp->fw_cap &= ~BNXT_FW_CAP_PTP_RTC;
>> - }
>> +
>> if (flags & FUNC_QCFG_RESP_FLAGS_RING_MONITOR_ENABLED)
>> bp->fw_cap |= BNXT_FW_CAP_RING_MONITOR;
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> index dcb09fbe4007..41e4bb7b8acb 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
>> @@ -2000,6 +2000,8 @@ struct bnxt {
>> u32 fw_dbg_cap;
>>
>> #define BNXT_NEW_RM(bp) ((bp)->fw_cap & BNXT_FW_CAP_NEW_RM)
>> +#define BNXT_PTP_RTC(bp) (!BNXT_MH(bp) && \
>> + ((bp)->fw_cap & BNXT_FW_CAP_PTP_RTC))
>> u32 hwrm_spec_code;
>> u16 hwrm_cmd_seq;
>> u16 hwrm_cmd_kong_seq;
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index 4ec8bba18cdd..99c1a53231aa 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -63,7 +63,7 @@ static int bnxt_ptp_settime(struct ptp_clock_info *ptp_info,
>> ptp_info);
>> u64 ns = timespec64_to_ns(ts);
>>
>> - if (ptp->bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>> + if (BNXT_PTP_RTC(ptp->bp))
>> return bnxt_ptp_cfg_settime(ptp->bp, ns);
>>
>> spin_lock_bh(&ptp->ptp_lock);
>> @@ -196,7 +196,7 @@ static int bnxt_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
>> struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
>> ptp_info);
>>
>> - if (ptp->bp->fw_cap & BNXT_FW_CAP_PTP_RTC)
>> + if (BNXT_PTP_RTC(ptp->bp))
>> return bnxt_ptp_adjphc(ptp, delta);
>>
>> spin_lock_bh(&ptp->ptp_lock);
>> @@ -205,34 +205,39 @@ static int bnxt_ptp_adjtime(struct ptp_clock_info *ptp_info, s64 delta)
>> return 0;
>> }
>>
>> +static int bnxt_ptp_adjfine_rtc(struct bnxt *bp, long scaled_ppm)
>> +{
>> + s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
>> + struct hwrm_port_mac_cfg_input *req;
>> + int rc;
>> +
>> + rc = hwrm_req_init(bp, req, HWRM_PORT_MAC_CFG);
>> + if (rc)
>> + return rc;
>> +
>> + req->ptp_freq_adj_ppb = cpu_to_le32(ppb);
>> + req->enables = cpu_to_le32(PORT_MAC_CFG_REQ_ENABLES_PTP_FREQ_ADJ_PPB);
>> + rc = hwrm_req_send(bp, req);
>> + if (rc)
>> + netdev_err(bp->dev,
>> + "ptp adjfine failed. rc = %d\n", rc);
>> + return rc;
>> +}
>> +
>> static int bnxt_ptp_adjfine(struct ptp_clock_info *ptp_info, long scaled_ppm)
>> {
>> struct bnxt_ptp_cfg *ptp = container_of(ptp_info, struct bnxt_ptp_cfg,
>> ptp_info);
>> - struct hwrm_port_mac_cfg_input *req;
>> struct bnxt *bp = ptp->bp;
>> - int rc = 0;
>>
>> - if (!(ptp->bp->fw_cap & BNXT_FW_CAP_PTP_RTC)) {
>> - spin_lock_bh(&ptp->ptp_lock);
>> - timecounter_read(&ptp->tc);
>> - ptp->cc.mult = adjust_by_scaled_ppm(ptp->cmult, scaled_ppm);
>> - spin_unlock_bh(&ptp->ptp_lock);
>> - } else {
>> - s32 ppb = scaled_ppm_to_ppb(scaled_ppm);
>> -
>> - rc = hwrm_req_init(bp, req, HWRM_PORT_MAC_CFG);
>> - if (rc)
>> - return rc;
>> + if (BNXT_PTP_RTC(ptp->bp))
>> + return bnxt_ptp_adjfine_rtc(bp, scaled_ppm);
>>
>> - req->ptp_freq_adj_ppb = cpu_to_le32(ppb);
>> - req->enables = cpu_to_le32(PORT_MAC_CFG_REQ_ENABLES_PTP_FREQ_ADJ_PPB);
>> - rc = hwrm_req_send(ptp->bp, req);
>> - if (rc)
>> - netdev_err(ptp->bp->dev,
>> - "ptp adjfine failed. rc = %d\n", rc);
>> - }
>> - return rc;
>> + spin_lock_bh(&ptp->ptp_lock);
>> + timecounter_read(&ptp->tc);
>> + ptp->cc.mult = adjust_by_scaled_ppm(ptp->cmult, scaled_ppm);
>> + spin_unlock_bh(&ptp->ptp_lock);
>> + return 0;
>> }
>>
>> void bnxt_ptp_pps_event(struct bnxt *bp, u32 data1, u32 data2)
>> @@ -879,7 +884,7 @@ int bnxt_ptp_init_rtc(struct bnxt *bp, bool phc_cfg)
>> u64 ns;
>> int rc;
>>
>> - if (!bp->ptp_cfg || !(bp->fw_cap & BNXT_FW_CAP_PTP_RTC))
>> + if (!bp->ptp_cfg || !BNXT_PTP_RTC(bp))
>> return -ENODEV;
>>
>> if (!phc_cfg) {
>> @@ -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);
>
> 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
>>
Powered by blists - more mailing lists