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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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