[PATCH net-next] net: fec: implement TSO descriptor cleanup

[PATCH net-next] net: fec: implement TSO descriptor cleanup

[Dheeraj Reddy Jonnalagadda]

Implement the TODO in fec_enet_txq_submit_tso() error path to properly
release buffer descriptors that were allocated during a failed TSO
operation. This prevents descriptor leaks when TSO operations fail
partway through.

The cleanup iterates from the starting descriptor to where the error
occurred, resetting the status and buffer address fields of each
descriptor.

Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index b2daed55bf6c..eff065010c9e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -913,7 +913,18 @@ static int fec_enet_txq_submit_tso(struct fec_enet_priv_tx_q *txq,
 	return 0;
 
 err_release:
-	/* TODO: Release all used data descriptors for TSO */
+	/* Release all used data descriptors for TSO */
+	struct bufdesc *tmp_bdp = txq->bd.cur;
+
+	while (tmp_bdp != bdp) {
+		tmp_bdp->cbd_sc = 0;
+		tmp_bdp->cbd_bufaddr = 0;
+		tmp_bdp->cbd_datlen = 0;
+		tmp_bdp = fec_enet_get_nextdesc(tmp_bdp, &txq->bd);
+	}
+
+	dev_kfree_skb_any(skb);
+
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH net-next] net: fec: implement TSO descriptor cleanup

[Simon Horman]

On Thu, Jan 16, 2025 at 06:39:20PM +0530, Dheeraj Reddy Jonnalagadda wrote:
> Implement the TODO in fec_enet_txq_submit_tso() error path to properly
> release buffer descriptors that were allocated during a failed TSO
> operation. This prevents descriptor leaks when TSO operations fail
> partway through.
> 
> The cleanup iterates from the starting descriptor to where the error
> occurred, resetting the status and buffer address fields of each
> descriptor.
> 
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index b2daed55bf6c..eff065010c9e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -913,7 +913,18 @@ static int fec_enet_txq_submit_tso(struct fec_enet_priv_tx_q *txq,
>  	return 0;
>  
>  err_release:
> -	/* TODO: Release all used data descriptors for TSO */
> +	/* Release all used data descriptors for TSO */
> +	struct bufdesc *tmp_bdp = txq->bd.cur;

Hi Dheeraj,

This is not a full review. But variable declarations
should appear at the top of the block they are in,
in this case that would be at the top of the function.

> +
> +	while (tmp_bdp != bdp) {
> +		tmp_bdp->cbd_sc = 0;
> +		tmp_bdp->cbd_bufaddr = 0;
> +		tmp_bdp->cbd_datlen = 0;
> +		tmp_bdp = fec_enet_get_nextdesc(tmp_bdp, &txq->bd);
> +	}
> +
> +	dev_kfree_skb_any(skb);
> +
>  	return ret;
>  }

-- 
pw-bot: changes-requested

RE: [PATCH net-next] net: fec: implement TSO descriptor cleanup

[Wei Fang]

> Implement the TODO in fec_enet_txq_submit_tso() error path to properly
> release buffer descriptors that were allocated during a failed TSO
> operation. This prevents descriptor leaks when TSO operations fail
> partway through.
> 
> The cleanup iterates from the starting descriptor to where the error
> occurred, resetting the status and buffer address fields of each
> descriptor.
> 
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index b2daed55bf6c..eff065010c9e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -913,7 +913,18 @@ static int fec_enet_txq_submit_tso(struct
> fec_enet_priv_tx_q *txq,
>  	return 0;
> 
>  err_release:
> -	/* TODO: Release all used data descriptors for TSO */
> +	/* Release all used data descriptors for TSO */
> +	struct bufdesc *tmp_bdp = txq->bd.cur;
> +
> +	while (tmp_bdp != bdp) {
> +		tmp_bdp->cbd_sc = 0;
> +		tmp_bdp->cbd_bufaddr = 0;
> +		tmp_bdp->cbd_datlen = 0;
> +		tmp_bdp = fec_enet_get_nextdesc(tmp_bdp, &txq->bd);
> +	}

There is still some cleanup to do.
1. If bufdesc_ex is used, we also need to clear it, such as ebdp->cbd_esc.
2. The data buffers have been mapped in fec_enet_txq_put_data_tso(),
I think we need to unmap them in the error path. But do not unmap
the TSO header buff, which is a DMA memory. Actually it is not necessary
for fec_enet_txq_put_hdr_tso() to call dma_map_single(). If you are
interested, you can add a separate patch to remove dma_map_single()
in fec_enet_txq_put_hdr_tso().

> +
> +	dev_kfree_skb_any(skb);
> +
>  	return ret;
>  }
> 
> --
> 2.34.1

Re: [PATCH net-next] net: fec: implement TSO descriptor cleanup

[kernel test robot]

Hi Dheeraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Dheeraj-Reddy-Jonnalagadda/net-fec-implement-TSO-descriptor-cleanup/20250116-211046
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250116130920.30984-1-dheeraj.linuxdev%40gmail.com
patch subject: [PATCH net-next] net: fec: implement TSO descriptor cleanup
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20250117/202501172328.IoOTB8n0-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250117/202501172328.IoOTB8n0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501172328.IoOTB8n0-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/freescale/fec_main.c:25:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2224:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/net/ethernet/freescale/fec_main.c:917:2: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
     917 |         struct bufdesc *tmp_bdp = txq->bd.cur;
         |         ^
   4 warnings generated.


vim +917 drivers/net/ethernet/freescale/fec_main.c

   835	
   836	static int fec_enet_txq_submit_tso(struct fec_enet_priv_tx_q *txq,
   837					   struct sk_buff *skb,
   838					   struct net_device *ndev)
   839	{
   840		struct fec_enet_private *fep = netdev_priv(ndev);
   841		int hdr_len, total_len, data_left;
   842		struct bufdesc *bdp = txq->bd.cur;
   843		struct tso_t tso;
   844		unsigned int index = 0;
   845		int ret;
   846	
   847		if (tso_count_descs(skb) >= fec_enet_get_free_txdesc_num(txq)) {
   848			dev_kfree_skb_any(skb);
   849			if (net_ratelimit())
   850				netdev_err(ndev, "NOT enough BD for TSO!\n");
   851			return NETDEV_TX_OK;
   852		}
   853	
   854		/* Protocol checksum off-load for TCP and UDP. */
   855		if (fec_enet_clear_csum(skb, ndev)) {
   856			dev_kfree_skb_any(skb);
   857			return NETDEV_TX_OK;
   858		}
   859	
   860		/* Initialize the TSO handler, and prepare the first payload */
   861		hdr_len = tso_start(skb, &tso);
   862	
   863		total_len = skb->len - hdr_len;
   864		while (total_len > 0) {
   865			char *hdr;
   866	
   867			index = fec_enet_get_bd_index(bdp, &txq->bd);
   868			data_left = min_t(int, skb_shinfo(skb)->gso_size, total_len);
   869			total_len -= data_left;
   870	
   871			/* prepare packet headers: MAC + IP + TCP */
   872			hdr = txq->tso_hdrs + index * TSO_HEADER_SIZE;
   873			tso_build_hdr(skb, hdr, &tso, data_left, total_len == 0);
   874			ret = fec_enet_txq_put_hdr_tso(txq, skb, ndev, bdp, index);
   875			if (ret)
   876				goto err_release;
   877	
   878			while (data_left > 0) {
   879				int size;
   880	
   881				size = min_t(int, tso.size, data_left);
   882				bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
   883				index = fec_enet_get_bd_index(bdp, &txq->bd);
   884				ret = fec_enet_txq_put_data_tso(txq, skb, ndev,
   885								bdp, index,
   886								tso.data, size,
   887								size == data_left,
   888								total_len == 0);
   889				if (ret)
   890					goto err_release;
   891	
   892				data_left -= size;
   893				tso_build_data(skb, &tso, size);
   894			}
   895	
   896			bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
   897		}
   898	
   899		/* Save skb pointer */
   900		txq->tx_buf[index].buf_p = skb;
   901	
   902		skb_tx_timestamp(skb);
   903		txq->bd.cur = bdp;
   904	
   905		/* Trigger transmission start */
   906		if (!(fep->quirks & FEC_QUIRK_ERR007885) ||
   907		    !readl(txq->bd.reg_desc_active) ||
   908		    !readl(txq->bd.reg_desc_active) ||
   909		    !readl(txq->bd.reg_desc_active) ||
   910		    !readl(txq->bd.reg_desc_active))
   911			writel(0, txq->bd.reg_desc_active);
   912	
   913		return 0;
   914	
   915	err_release:
   916		/* Release all used data descriptors for TSO */
 > 917		struct bufdesc *tmp_bdp = txq->bd.cur;
   918	
   919		while (tmp_bdp != bdp) {
   920			tmp_bdp->cbd_sc = 0;
   921			tmp_bdp->cbd_bufaddr = 0;
   922			tmp_bdp->cbd_datlen = 0;
   923			tmp_bdp = fec_enet_get_nextdesc(tmp_bdp, &txq->bd);
   924		}
   925	
   926		dev_kfree_skb_any(skb);
   927	
   928		return ret;
   929	}
   930	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next] net: fec: implement TSO descriptor cleanup

[Dheeraj Reddy Jonnalagadda]

On Fri, Jan 17, 2025 at 02:47:33AM +0000, Wei Fang wrote:
> > Implement the TODO in fec_enet_txq_submit_tso() error path to properly
> > release buffer descriptors that were allocated during a failed TSO
> > operation. This prevents descriptor leaks when TSO operations fail
> > partway through.
> > 
> > The cleanup iterates from the starting descriptor to where the error
> > occurred, resetting the status and buffer address fields of each
> > descriptor.
> > 
> > Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index b2daed55bf6c..eff065010c9e 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -913,7 +913,18 @@ static int fec_enet_txq_submit_tso(struct
> > fec_enet_priv_tx_q *txq,
> >  	return 0;
> > 
> >  err_release:
> > -	/* TODO: Release all used data descriptors for TSO */
> > +	/* Release all used data descriptors for TSO */
> > +	struct bufdesc *tmp_bdp = txq->bd.cur;
> > +
> > +	while (tmp_bdp != bdp) {
> > +		tmp_bdp->cbd_sc = 0;
> > +		tmp_bdp->cbd_bufaddr = 0;
> > +		tmp_bdp->cbd_datlen = 0;
> > +		tmp_bdp = fec_enet_get_nextdesc(tmp_bdp, &txq->bd);
> > +	}
> 
> There is still some cleanup to do.
> 1. If bufdesc_ex is used, we also need to clear it, such as ebdp->cbd_esc.
> 2. The data buffers have been mapped in fec_enet_txq_put_data_tso(),
> I think we need to unmap them in the error path. But do not unmap
> the TSO header buff, which is a DMA memory. Actually it is not necessary
> for fec_enet_txq_put_hdr_tso() to call dma_map_single(). If you are
> interested, you can add a separate patch to remove dma_map_single()
> in fec_enet_txq_put_hdr_tso().
> 
> > +
> > +	dev_kfree_skb_any(skb);
> > +
> >  	return ret;
> >  }
> > 
> > --
> > 2.34.1
> 
Hello Simon and Fang,

Thank you you valuable feedback. I will incorporate the suggestions and 
send the updated patch.

-Dheeraj