[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACKFLikv==pNi2i9FbpQ-qOJofPxq6eoj5j_9bA_bqRBj+NV2Q@mail.gmail.com>
Date: Fri, 28 Jun 2024 10:37:19 -0700
From: Michael Chan <michael.chan@...adcom.com>
To: Simon Horman <horms@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, pavan.chebbi@...adcom.com,
andrew.gospodarek@...adcom.com, richardcochran@...il.com
Subject: Re: [PATCH net-next 09/10] bnxt_en: Increase the max total
outstanding PTP TX packets to 4
On Fri, Jun 28, 2024 at 10:03 AM Simon Horman <horms@...nel.org> wrote:
>
> On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index ed2bbdf6b25f..0867861c14bd 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -457,8 +457,8 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > unsigned int length, pad = 0;
> > u32 len, free_size, vlan_tag_flags, cfa_action, flags;
> > struct bnxt_ptp_cfg *ptp = bp->ptp_cfg;
> > - u16 prod, last_frag;
> > struct pci_dev *pdev = bp->pdev;
> > + u16 prod, last_frag, txts_prod;
> > struct bnxt_tx_ring_info *txr;
> > struct bnxt_sw_tx_bd *tx_buf;
> > __le32 lflags = 0;
> > @@ -526,11 +526,19 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > if (!bnxt_ptp_parse(skb, &seq_id, &hdr_off)) {
> > if (vlan_tag_flags)
> > hdr_off += VLAN_HLEN;
> > - ptp->txts_req.tx_seqid = seq_id;
> > - ptp->txts_req.tx_hdr_off = hdr_off;
> > lflags |= cpu_to_le32(TX_BD_FLAGS_STAMP);
> > tx_buf->is_ts_pkt = 1;
> > skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
> > +
> > + spin_lock_bh(&ptp->ptp_tx_lock);
> > + txts_prod = ptp->txts_prod;
> > + ptp->txts_prod = NEXT_TXTS(txts_prod);
> > + spin_unlock_bh(&ptp->ptp_tx_lock);
> > +
> > + ptp->txts_req[txts_prod].tx_seqid = seq_id;
> > + ptp->txts_req[txts_prod].tx_hdr_off = hdr_off;
> > + tx_buf->txts_prod = txts_prod;
> > +
> > } else {
> > atomic_inc(&bp->ptp_cfg->tx_avail);
> > }
> > @@ -770,7 +778,9 @@ static netdev_tx_t bnxt_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > tx_kick_pending:
> > if (BNXT_TX_PTP_IS_SET(lflags)) {
> > atomic64_inc(&bp->ptp_cfg->stats.ts_err);
> > - atomic_inc(&bp->ptp_cfg->tx_avail);
> > + if (!(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP))
> > + /* set SKB to err so PTP worker will clean up */
> > + ptp->txts_req[txts_prod].tx_skb = ERR_PTR(-EIO);
>
> Hi Michael
>
> Sparse complains that previously it was assumed that ptp could be NULL,
> but here it is accessed without checking for that.
>
> Perhaps it can't occur, but my brief check leads me to think it might.
Simon, thanks for the review. The key is this if statement:
if (BNXT_TX_PTP_IS_SET(lflags))
This if statement is true if the lflags have the TX_BD_FLAGS_STAMP
set. This flag is set only if ptp is valid because this flag tells
the hardware to take the timestamp.
>
> On line 488 there is the following:
>
> if (unlikely(ipv6_hopopt_jumbo_remove(skb)))
> goto tx_free;
>
> Which will lead to the code in the hunk above.
The lflags will not have the TX_BD_FLAGS_STAMP flag set if we jump from here.
>
> Then on line 513 there is a check for ptp being NULL:
>
>
> if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) && ptp &&
> ptp->tx_tstamp_en) {
>
> And ptp is not set between lines 488 and 513.
>
>
> Sparse also complains that txts_prod may be used uninitaialised.
> This also seems to be a valid concern as it does seem to be the case
> on line 488.
Same explanation for txts_prod. txts_prod will always be set if
lflags has TX_BD_FLAGS_STAMP set and this condition is false:
(bp->fw_cap & BNXT_FW_CAP_TX_TS_CMP)
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)
Powered by blists - more mailing lists