[PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk_read

[PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk_read

[Vadim Fedorenko]

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);
 	return ns;
 }
 
-- 
2.43.5

Re: [PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk_read

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 3388 bytes --]

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
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk_read

[Vadim Fedorenko]

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
>>

Re: [PATCH net-next] bnxt_en: add unlocked version of bnxt_refclk_read

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

> >> -       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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]