Re: [PATCH net-next 09/10] bnxt_en: Increase the max total outstanding PTP TX packets to 4

[Simon Horman <horms@kernel.org>]

On Fri, Jun 28, 2024 at 06:03:18PM +0100, Simon Horman wrote:
> On Wed, Jun 26, 2024 at 09:43:06AM -0700, Michael Chan wrote:
> > From: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > 
> > Start accepting up to 4 TX TS requests on BCM5750X (P5) chips.
> > These PTP TX packets will be queued in the ptp->txts_req[] array
> > waiting for the TX timestamp to complete.  The entries in the
> > array will be managed by a producer and consumer index.  The
> > producer index is updated under spinlock since multiple TX rings
> > can try to send PTP packets at the same time.
> > 
> > Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> 
> ...
> 
> > 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.
> 
> 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.
> 
> 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.
> 
> >  	}
> >  	if (txr->kick_pending)
> >  		bnxt_txr_db_kick(bp, txr, txr->tx_prod);
> 
> ...

Sorry, in my previous email I mentioned Sparse.
But I should have said Smatch.