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

[Simon Horman <horms@kernel.org>]

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);

...