Re: [PATCH net v4] bnxt_en: improve TX timestamping FIFO configuration

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

On 30/04/2025 17:43, Taehee Yoo wrote:
> On Wed, Apr 30, 2025 at 11:55 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 30/04/2025 13:59, Taehee Yoo wrote:
>>> On Thu, Apr 24, 2025 at 10:11 PM Vadim Fedorenko <vadfed@meta.com> wrote:
>>>>
>>>
>>> Hi Vadim,
>>> Thanks for this work!
>>>
>>>> Reconfiguration of netdev may trigger close/open procedure which can
>>>> break FIFO status by adjusting the amount of empty slots for TX
>>>> timestamps. But it is not really needed because timestamps for the
>>>> packets sent over the wire still can be retrieved. On the other side,
>>>> during netdev close procedure any skbs waiting for TX timestamps can be
>>>> leaked because there is no cleaning procedure called. Free skbs waiting
>>>> for TX timestamps when closing netdev.
>>>>
>>>> Fixes: 8aa2a79e9b95 ("bnxt_en: Increase the max total outstanding PTP TX packets to 4")
>>>> Reviewed-by: Michael Chan <michael.chan@broadcom.com>
>>>> Reviewed-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
>>>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>>>> ---
>>>> v3 -> v4:
>>>> * actually remove leftover unused variable in bnxt_ptp_clear()
>>>> (v3 was not committed before preparing unfortunately)
>>>> v2 -> v3:
>>>> * remove leftover unused variable in bnxt_ptp_clear()
>>>> v1 -> v2:
>>>> * move clearing of TS skbs to bnxt_free_tx_skbs
>>>> * remove spinlock as no TX is possible after bnxt_tx_disable()
>>>> * remove extra FIFO clearing in bnxt_ptp_clear()
>>>> ---
>>>>    drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  5 ++--
>>>>    drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 29 ++++++++++++++-----
>>>>    drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
>>>>    3 files changed, 25 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>>> index c8e3468eee61..2c8e2c19d854 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>>>> @@ -3414,6 +3414,9 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
>>>>
>>>>                   bnxt_free_one_tx_ring_skbs(bp, txr, i);
>>>>           }
>>>> +
>>>> +       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
>>>> +               bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
>>>>    }
>>>>
>>>>    static void bnxt_free_one_rx_ring(struct bnxt *bp, struct bnxt_rx_ring_info *rxr)
>>>> @@ -12797,8 +12800,6 @@ static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
>>>>           /* VF-reps may need to be re-opened after the PF is re-opened */
>>>>           if (BNXT_PF(bp))
>>>>                   bnxt_vf_reps_open(bp);
>>>> -       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
>>>> -               WRITE_ONCE(bp->ptp_cfg->tx_avail, BNXT_MAX_TX_TS);
>>>>           bnxt_ptp_init_rtc(bp, true);
>>>>           bnxt_ptp_cfg_tstamp_filters(bp);
>>>>           if (BNXT_SUPPORTS_MULTI_RSS_CTX(bp))
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> index 2d4e19b96ee7..0669d43472f5 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>>>> @@ -794,6 +794,27 @@ static long bnxt_ptp_ts_aux_work(struct ptp_clock_info *ptp_info)
>>>>           return HZ;
>>>>    }
>>>>
>>>> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp)
>>>> +{
>>>> +       struct bnxt_ptp_tx_req *txts_req;
>>>> +       u16 cons = ptp->txts_cons;
>>>> +
>>>> +       /* make sure ptp aux worker finished with
>>>> +        * possible BNXT_STATE_OPEN set
>>>> +        */
>>>> +       ptp_cancel_worker_sync(ptp->ptp_clock);
>>>> +
>>>> +       ptp->tx_avail = BNXT_MAX_TX_TS;
>>>> +       while (cons != ptp->txts_prod) {
>>>> +               txts_req = &ptp->txts_req[cons];
>>>> +               if (!IS_ERR_OR_NULL(txts_req->tx_skb))
>>>> +                       dev_kfree_skb_any(txts_req->tx_skb);
>>>> +               cons = NEXT_TXTS(cons);
>>>> +       }
>>>> +       ptp->txts_cons = cons;
>>>> +       ptp_schedule_worker(ptp->ptp_clock, 0);
>>>> +}
>>>> +
>>>>    int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod)
>>>>    {
>>>>           spin_lock_bh(&ptp->ptp_tx_lock);
>>>> @@ -1105,7 +1126,6 @@ int bnxt_ptp_init(struct bnxt *bp)
>>>>    void bnxt_ptp_clear(struct bnxt *bp)
>>>>    {
>>>>           struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
>>>> -       int i;
>>>>
>>>>           if (!ptp)
>>>>                   return;
>>>> @@ -1117,12 +1137,5 @@ void bnxt_ptp_clear(struct bnxt *bp)
>>>>           kfree(ptp->ptp_info.pin_config);
>>>>           ptp->ptp_info.pin_config = NULL;
>>>>
>>>> -       for (i = 0; i < BNXT_MAX_TX_TS; i++) {
>>>> -               if (ptp->txts_req[i].tx_skb) {
>>>> -                       dev_kfree_skb_any(ptp->txts_req[i].tx_skb);
>>>> -                       ptp->txts_req[i].tx_skb = NULL;
>>>> -               }
>>>> -       }
>>>> -
>>>>           bnxt_unmap_ptp_regs(bp);
>>>>    }
>>>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>>>> index a95f05e9c579..0481161d26ef 100644
>>>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>>>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h
>>>> @@ -162,6 +162,7 @@ int bnxt_ptp_cfg_tstamp_filters(struct bnxt *bp);
>>>>    void bnxt_ptp_reapply_pps(struct bnxt *bp);
>>>>    int bnxt_hwtstamp_set(struct net_device *dev, struct ifreq *ifr);
>>>>    int bnxt_hwtstamp_get(struct net_device *dev, struct ifreq *ifr);
>>>> +void bnxt_ptp_free_txts_skbs(struct bnxt_ptp_cfg *ptp);
>>>>    int bnxt_ptp_get_txts_prod(struct bnxt_ptp_cfg *ptp, u16 *prod);
>>>>    void bnxt_get_tx_ts_p5(struct bnxt *bp, struct sk_buff *skb, u16 prod);
>>>>    int bnxt_get_rx_ts_p5(struct bnxt *bp, u64 *ts, u32 pkt_ts);
>>>> --
>>>> 2.47.1
>>>>
>>>>
>>>
>>> I’ve encountered a kernel panic that I think is related to this patch.
>>> Could you please investigate it?
>>>
>>> Reproducer:
>>>       ip link set $interface up
>>>       modprobe -rv bnxt_en
>>>
>>
>> Hi Taehee!
>>
>> Yeah, looks like there are some issues on the remove path.
>> Could you please test the diff which may fix the problem:
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index 78e496b0ec26..86a5de44b6f3 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -16006,8 +16006,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
>>
>>           bnxt_rdma_aux_device_del(bp);
>>
>> -       bnxt_ptp_clear(bp);
>>           unregister_netdev(dev);
>> +       bnxt_ptp_clear(bp);
>>
>>           bnxt_rdma_aux_device_uninit(bp);
>>
> 
> Thanks! It seems to fix the issue.
> The above reproducer can't reproduce a kernel panic.

Great, thanks, I'll post a proper patch soon