[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <90aea4dd-cb37-4dab-99ef-d45915514787@linux.dev>
Date: Thu, 7 Nov 2024 11:17:34 +0000
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Pavan Chebbi <pavan.chebbi@...adcom.com>,
Vadim Fedorenko <vadfed@...a.com>
Cc: Michael Chan <michael.chan@...adcom.com>, Jakub Kicinski
<kuba@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
Paolo Abeni <pabeni@...hat.com>, "David S. Miller" <davem@...emloft.net>,
netdev@...r.kernel.org, Richard Cochran <richardcochran@...il.com>
Subject: Re: [PATCH net-next] bnxt_en: add unlocked version of
bnxt_refclk_read
On 07/11/2024 04:30, Pavan Chebbi wrote:
> On Thu, Nov 7, 2024 at 3:02 AM Vadim Fedorenko <vadfed@...a.com> wrote:
>>
>> Serialization of PHC read with FW reset mechanism uses ptp_lock which
>> also protects timecounter updates. This means we cannot grab it when
>> called from bnxt_cc_read(). Let's move locking into different function.
>>
>> Fixes: 6c0828d00f07 ("bnxt_en: replace PTP spinlock with seqlock")
>> Signed-off-by: Vadim Fedorenko <vadfed@...a.com>
>> ---
>> drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 29 ++++++++++++-------
>> 1 file changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index f74afdab4f7d..5395f125b601 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -73,19 +73,15 @@ static int bnxt_ptp_settime(struct ptp_clock_info *ptp_info,
>> return 0;
>> }
>>
>> -static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
>> - u64 *ns)
>> +/* Caller holds ptp_lock */
>> +static int __bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
>> + u64 *ns)
>> {
>> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>> u32 high_before, high_now, low;
>> - unsigned long flags;
>>
>> - /* We have to serialize reg access and FW reset */
>> - read_seqlock_excl_irqsave(&ptp->ptp_lock, flags);
>> - if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
>> - read_sequnlock_excl_irqrestore(&ptp->ptp_lock, flags);
>> + if (test_bit(BNXT_STATE_IN_FW_RESET, &bp->state))
>> return -EIO;
>> - }
>>
>> high_before = readl(bp->bar0 + ptp->refclk_mapped_regs[1]);
>> ptp_read_system_prets(sts);
>> @@ -97,12 +93,25 @@ static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
>> low = readl(bp->bar0 + ptp->refclk_mapped_regs[0]);
>> ptp_read_system_postts(sts);
>> }
>> - read_sequnlock_excl_irqrestore(&ptp->ptp_lock, flags);
>> *ns = ((u64)high_now << 32) | low;
>>
>> return 0;
>> }
>>
>> +static int bnxt_refclk_read(struct bnxt *bp, struct ptp_system_timestamp *sts,
>> + u64 *ns)
>> +{
>> + struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>> + unsigned long flags;
>> + int rc;
>> +
>> + /* We have to serialize reg access and FW reset */
>> + read_seqlock_excl_irqsave(&ptp->ptp_lock, flags);
>> + rc = __bnxt_refclk_read(bp, sts, ns);
>> + read_sequnlock_excl_irqrestore(&ptp->ptp_lock, flags);
>> + return rc;
>> +}
>> +
>> static void bnxt_ptp_get_current_time(struct bnxt *bp)
>> {
>> struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>> @@ -674,7 +683,7 @@ static u64 bnxt_cc_read(const struct cyclecounter *cc)
>> struct bnxt_ptp_cfg *ptp = container_of(cc, struct bnxt_ptp_cfg, cc);
>> u64 ns = 0;
>>
>> - bnxt_refclk_read(ptp->bp, NULL, &ns);
>> + __bnxt_refclk_read(ptp->bp, NULL, &ns);
>
> With this change, bnxt_cc_read() is executed without protection during
> timecounter_init(), right?
> I think we should hold the ptp_lock inside bnxt_ptp_timecounter_init()
> just like we do in bnxt_ptp_init_rtc()
Well, yes, that's correct. Technically we have to hold this lock (and I
will add it in v2), but if think a bit wider, do we expect
bnxt_fw_reset()/bnxt_force_fw_reset() to be called during device init
phase? If yes, we have proper time frame between bnxt_ptp_cfg allocation
in __bnxt_hwrm_ptp_qcfg() (which assigns it to bp->ptp) and spinlock
initialization in bnxt_ptp_init(), during which spinlock must not be
accessed. And if we imagine the situation when fw_reset request can be
initiated during initialization, the next flow can happen:
CPU0: CPU1:
__bnxt_hwrm_ptp_qcfg()
ptp_cfg = kzalloc()
bp->ptp = ptp_cfg
bnxt_force_fw_reset()
if (bp->ptp)
spin_lock_irqsave(bp->ptp->ptp_lock)
bnxt_ptp_init()
spinlock_init(ptp_lock)
So we either should not have an option of resetting FW during init
phase (and then there will be no need to use lock), or we have to
re-think FW reset serialization completley. WDYT?
>> return ns;
>> }
>>
>> --
>> 2.43.5
>>
Powered by blists - more mailing lists