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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACKFLi=Hc9apfJTzJFGY7jOK6vJKvUbOM=zXhnYfNi8xpiHuJg@mail.gmail.com>
Date: Wed, 16 Apr 2025 10:54:07 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Cc: Pavan Chebbi <pavan.chebbi@...adcom.com>, Jakub Kicinski <kuba@...nel.org>, 
	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] bnxt_en: improve TX timestamping FIFO configuration

On Wed, Apr 16, 2025 at 10:35 AM Vadim Fedorenko
<vadim.fedorenko@...ux.dev> wrote:
>
> On 16/04/2025 18:03, Michael Chan wrote:
> > On Wed, Apr 16, 2025 at 8:01 AM Vadim Fedorenko <vadfed@...a.com> wrote:
> >> 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() ?

Yes, move these 2 lines to the end of bnxt_free_tx_skbs().  I think it
just makes a little more sense since these are transmit 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.
>

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

We generally iterate over all ring indices in the cleanup code.  For
the PTP slots, I think it is not necessary.  Any valid SKBs should be
between the consumer and producer indices.  So, either way is fine
with me.  There are only 4 slots after all.  Thanks.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4196 bytes)