| Message ID | 20250116130920.30984-1-dheeraj.linuxdev@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next] net: fec: implement TSO descriptor cleanup | expand |
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; > }
> 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
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
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
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; }
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(-)