| Message ID | 20241106213203.3997563-1-vadfed@meta.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bnxt_en: add unlocked version of bnxt_refclk_read | expand |
On Thu, Nov 7, 2024 at 3:02 AM Vadim Fedorenko <vadfed@meta.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@meta.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() > return ns; > } > > -- > 2.43.5 >
On 07/11/2024 04:30, Pavan Chebbi wrote: > On Thu, Nov 7, 2024 at 3:02 AM Vadim Fedorenko <vadfed@meta.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@meta.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 >>
> >> - 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? > I don't disagree. Since bnxt_ptp_init() is happening at the probe time, absent async event queue and devlink infra, in my mind we can safely assume we won't encounter a race with fw_reset. The holding of lock is more for being theoretically correct, like you pointed out.
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); return ns; }
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@meta.com> --- drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 29 ++++++++++++------- 1 file changed, 19 insertions(+), 10 deletions(-)