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

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

On 16/04/2025 18:03, Michael Chan wrote:
> On Wed, Apr 16, 2025 at 8:01 AM Vadim Fedorenko <vadfed@meta.com> wrote:
>>
>> 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")
>> Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
>> ---
>>   drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  4 ++--
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c | 23 +++++++++++++++++++
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.h |  1 +
>>   3 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> index c8e3468eee61..45d178586316 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> @@ -3517,6 +3517,8 @@ static void bnxt_free_skbs(struct bnxt *bp)
>>   {
>>          bnxt_free_tx_skbs(bp);
>>          bnxt_free_rx_skbs(bp);
>> +       if (bp->ptp_cfg && !(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
>> +               bnxt_ptp_free_txts_skbs(bp->ptp_cfg);
> 
> Since these are TX SKBs, it's slightly more logical if we put this in
> bnxt_free_tx_skbs().

Do you mean to move this chunk to bnxt_free_tx_skbs() ?
I put it here because the driver has 3 different FIFOs to keep SKBs,
and it's logical to move PTP FIFO free function out of TX part..
But have no strong opinion.

>>   }
>>
>>   static void bnxt_init_ctx_mem(struct bnxt_ctx_mem_type *ctxm, void *p, int len)
> 
>> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> index 2d4e19b96ee7..39dc4f1f651a 100644
>> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ptp.c
>> @@ -794,6 +794,29 @@ 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);
>> +
>> +       spin_lock_bh(&ptp->ptp_tx_lock);
> 
> I think the spinlock is not needed because bnxt_tx_disable() should
> have been called already in the close path.

Hmm ... ok, yeah, TX won't be woken up at that moment. I'll remove it.

> 
>> +       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);
> 
> I think we can remove the similar code we have in bnxt_ptp_clear().
> We should always go through this path before bnxt_ptp_clear().

The difference with bnxt_ptp_clear() code is that this one clears SKBs
waiting in the queue according to consumer/producer pointers while
bnxt_ptp_clear() iterates over all slots, a bit more on safe side. 
Should I adjust this part to check all slots before removing
bnxt_ptp_clear() FIFO manipulations?

I believe in the normal way of things there should be no need to iterate
over all slots, but maybe you think of some conditions when we have to
check all slots?

> 
> Thanks for the patch.