lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 27 Sep 2022 14:47:37 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Ioana Ciornei <ioana.ciornei@....com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>, bpf@...r.kernel.org,
        Robert-Ionut Alexa <robert-ionut.alexa@....com>
Subject: Re: [PATCH net-next v2 03/12] net: dpaa2-eth: add support for
 multiple buffer pools per DPNI

On Fri, 2022-09-23 at 18:45 +0300, Ioana Ciornei wrote:
> From: Robert-Ionut Alexa <robert-ionut.alexa@....com>
> 
> This patch allows the configuration of multiple buffer pools associated
> with a single DPNI object, each distinct DPBP object not necessarily
> shared among all queues.
> The user can interogate both the number of buffer pools and the buffer
> count in each buffer pool by using the .get_ethtool_stats() callback.
> 
> Signed-off-by: Robert-Ionut Alexa <robert-ionut.alexa@....com>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> ---
> Changes in v2:
>  - Export dpaa2_eth_allocate_dpbp/dpaa2_eth_free_dpbp in this patch to
>    avoid a build warning. The functions will be used in next patches.
> 
>  .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  | 188 ++++++++++++------
>  .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  26 ++-
>  .../ethernet/freescale/dpaa2/dpaa2-ethtool.c  |  15 +-
>  3 files changed, 162 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 75d51572693d..aa93ed339b63 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>  /* Copyright 2014-2016 Freescale Semiconductor Inc.
> - * Copyright 2016-2020 NXP
> + * Copyright 2016-2022 NXP
>   */
>  #include <linux/init.h>
>  #include <linux/module.h>
> @@ -304,7 +304,7 @@ static void dpaa2_eth_recycle_buf(struct dpaa2_eth_priv *priv,
>  	if (ch->recycled_bufs_cnt < DPAA2_ETH_BUFS_PER_CMD)
>  		return;
>  
> -	while ((err = dpaa2_io_service_release(ch->dpio, priv->bpid,
> +	while ((err = dpaa2_io_service_release(ch->dpio, ch->bp->bpid,
>  					       ch->recycled_bufs,
>  					       ch->recycled_bufs_cnt)) == -EBUSY) {
>  		if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES)
> @@ -1631,7 +1631,7 @@ static int dpaa2_eth_set_tx_csum(struct dpaa2_eth_priv *priv, bool enable)
>   * to the specified buffer pool
>   */
>  static int dpaa2_eth_add_bufs(struct dpaa2_eth_priv *priv,
> -			      struct dpaa2_eth_channel *ch, u16 bpid)
> +			      struct dpaa2_eth_channel *ch)
>  {
>  	struct device *dev = priv->net_dev->dev.parent;
>  	u64 buf_array[DPAA2_ETH_BUFS_PER_CMD];
> @@ -1663,12 +1663,12 @@ static int dpaa2_eth_add_bufs(struct dpaa2_eth_priv *priv,
>  		trace_dpaa2_eth_buf_seed(priv->net_dev, page_address(page),
>  					 DPAA2_ETH_RX_BUF_RAW_SIZE,
>  					 addr, priv->rx_buf_size,
> -					 bpid);
> +					 ch->bp->bpid);
>  	}
>  
>  release_bufs:
>  	/* In case the portal is busy, retry until successful */
> -	while ((err = dpaa2_io_service_release(ch->dpio, bpid,
> +	while ((err = dpaa2_io_service_release(ch->dpio, ch->bp->bpid,
>  					       buf_array, i)) == -EBUSY) {
>  		if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES)
>  			break;
> @@ -1697,39 +1697,59 @@ static int dpaa2_eth_add_bufs(struct dpaa2_eth_priv *priv,
>  	return 0;
>  }
>  
> -static int dpaa2_eth_seed_pool(struct dpaa2_eth_priv *priv, u16 bpid)
> +static int dpaa2_eth_seed_pool(struct dpaa2_eth_priv *priv,
> +			       struct dpaa2_eth_channel *ch)
>  {
> -	int i, j;
> +	int i;
>  	int new_count;
>  
> -	for (j = 0; j < priv->num_channels; j++) {
> -		for (i = 0; i < DPAA2_ETH_NUM_BUFS;
> -		     i += DPAA2_ETH_BUFS_PER_CMD) {
> -			new_count = dpaa2_eth_add_bufs(priv, priv->channel[j], bpid);
> -			priv->channel[j]->buf_count += new_count;
> +	for (i = 0; i < DPAA2_ETH_NUM_BUFS; i += DPAA2_ETH_BUFS_PER_CMD) {
> +		new_count = dpaa2_eth_add_bufs(priv, ch);
> +		ch->buf_count += new_count;
>  
> -			if (new_count < DPAA2_ETH_BUFS_PER_CMD) {
> -				return -ENOMEM;
> -			}
> -		}
> +		if (new_count < DPAA2_ETH_BUFS_PER_CMD)
> +			return -ENOMEM;
>  	}
>  
>  	return 0;
>  }
>  
> +static void dpaa2_eth_seed_pools(struct dpaa2_eth_priv *priv)
> +{
> +	struct net_device *net_dev = priv->net_dev;
> +	struct dpaa2_eth_channel *channel;
> +	int i, err = 0;
> +
> +	for (i = 0; i < priv->num_channels; i++) {
> +		channel = priv->channel[i];
> +
> +		err = dpaa2_eth_seed_pool(priv, channel);
> +
> +		/* Not much to do; the buffer pool, though not filled up,
> +		 * may still contain some buffers which would enable us
> +		 * to limp on.
> +		 */
> +		if (err)
> +			netdev_err(net_dev, "Buffer seeding failed for DPBP %d (bpid=%d)\n",
> +				   channel->bp->dev->obj_desc.id,
> +				   channel->bp->bpid);
> +	}
> +}
> +
>  /*
> - * Drain the specified number of buffers from the DPNI's private buffer pool.
> + * Drain the specified number of buffers from one of the DPNI's private buffer
> + * pools.
>   * @count must not exceeed DPAA2_ETH_BUFS_PER_CMD
>   */
> -static void dpaa2_eth_drain_bufs(struct dpaa2_eth_priv *priv, int count)
> +static void dpaa2_eth_drain_bufs(struct dpaa2_eth_priv *priv, int bpid,
> +				 int count)
>  {
>  	u64 buf_array[DPAA2_ETH_BUFS_PER_CMD];
>  	int retries = 0;
>  	int ret;
>  
>  	do {
> -		ret = dpaa2_io_service_acquire(NULL, priv->bpid,
> -					       buf_array, count);
> +		ret = dpaa2_io_service_acquire(NULL, bpid, buf_array, count);
>  		if (ret < 0) {
>  			if (ret == -EBUSY &&
>  			    retries++ < DPAA2_ETH_SWP_BUSY_RETRIES)
> @@ -1742,23 +1762,35 @@ static void dpaa2_eth_drain_bufs(struct dpaa2_eth_priv *priv, int count)
>  	} while (ret);
>  }
>  
> -static void dpaa2_eth_drain_pool(struct dpaa2_eth_priv *priv)
> +static void dpaa2_eth_drain_pool(struct dpaa2_eth_priv *priv, int bpid)
>  {
>  	int i;
>  
> -	dpaa2_eth_drain_bufs(priv, DPAA2_ETH_BUFS_PER_CMD);
> -	dpaa2_eth_drain_bufs(priv, 1);
> +	/* Drain the buffer pool */
> +	dpaa2_eth_drain_bufs(priv, bpid, DPAA2_ETH_BUFS_PER_CMD);
> +	dpaa2_eth_drain_bufs(priv, bpid, 1);
>  
> +	/* Setup to zero the buffer count of all channels which were
> +	 * using this buffer pool.
> +	 */
>  	for (i = 0; i < priv->num_channels; i++)
> -		priv->channel[i]->buf_count = 0;
> +		if (priv->channel[i]->bp->bpid == bpid)
> +			priv->channel[i]->buf_count = 0;
> +}
> +
> +static void dpaa2_eth_drain_pools(struct dpaa2_eth_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < priv->num_bps; i++)
> +		dpaa2_eth_drain_pool(priv, priv->bp[i]->bpid);
>  }
>  
>  /* Function is called from softirq context only, so we don't need to guard
>   * the access to percpu count
>   */
>  static int dpaa2_eth_refill_pool(struct dpaa2_eth_priv *priv,
> -				 struct dpaa2_eth_channel *ch,
> -				 u16 bpid)
> +				 struct dpaa2_eth_channel *ch)
>  {
>  	int new_count;
>  
> @@ -1766,7 +1798,7 @@ static int dpaa2_eth_refill_pool(struct dpaa2_eth_priv *priv,
>  		return 0;
>  
>  	do {
> -		new_count = dpaa2_eth_add_bufs(priv, ch, bpid);
> +		new_count = dpaa2_eth_add_bufs(priv, ch);
>  		if (unlikely(!new_count)) {
>  			/* Out of memory; abort for now, we'll try later on */
>  			break;
> @@ -1848,7 +1880,7 @@ static int dpaa2_eth_poll(struct napi_struct *napi, int budget)
>  			break;
>  
>  		/* Refill pool if appropriate */
> -		dpaa2_eth_refill_pool(priv, ch, priv->bpid);
> +		dpaa2_eth_refill_pool(priv, ch);
>  
>  		store_cleaned = dpaa2_eth_consume_frames(ch, &fq);
>  		if (store_cleaned <= 0)
> @@ -2047,15 +2079,7 @@ static int dpaa2_eth_open(struct net_device *net_dev)
>  	struct dpaa2_eth_priv *priv = netdev_priv(net_dev);
>  	int err;
>  
> -	err = dpaa2_eth_seed_pool(priv, priv->bpid);
> -	if (err) {
> -		/* Not much to do; the buffer pool, though not filled up,
> -		 * may still contain some buffers which would enable us
> -		 * to limp on.
> -		 */
> -		netdev_err(net_dev, "Buffer seeding failed for DPBP %d (bpid=%d)\n",
> -			   priv->dpbp_dev->obj_desc.id, priv->bpid);
> -	}
> +	dpaa2_eth_seed_pools(priv);
>  
>  	if (!dpaa2_eth_is_type_phy(priv)) {
>  		/* We'll only start the txqs when the link is actually ready;
> @@ -2088,7 +2112,7 @@ static int dpaa2_eth_open(struct net_device *net_dev)
>  
>  enable_err:
>  	dpaa2_eth_disable_ch_napi(priv);
> -	dpaa2_eth_drain_pool(priv);
> +	dpaa2_eth_drain_pools(priv);
>  	return err;
>  }
>  
> @@ -2193,7 +2217,7 @@ static int dpaa2_eth_stop(struct net_device *net_dev)
>  	dpaa2_eth_disable_ch_napi(priv);
>  
>  	/* Empty the buffer pool */
> -	dpaa2_eth_drain_pool(priv);
> +	dpaa2_eth_drain_pools(priv);
>  
>  	/* Empty the Scatter-Gather Buffer cache */
>  	dpaa2_eth_sgt_cache_drain(priv);
> @@ -3204,13 +3228,14 @@ static void dpaa2_eth_setup_fqs(struct dpaa2_eth_priv *priv)
>  	dpaa2_eth_set_fq_affinity(priv);
>  }
>  
> -/* Allocate and configure one buffer pool for each interface */
> -static int dpaa2_eth_setup_dpbp(struct dpaa2_eth_priv *priv)
> +/* Allocate and configure a buffer pool */
> +struct dpaa2_eth_bp *dpaa2_eth_allocate_dpbp(struct dpaa2_eth_priv *priv)
>  {
> -	int err;
> -	struct fsl_mc_device *dpbp_dev;
>  	struct device *dev = priv->net_dev->dev.parent;
> +	struct fsl_mc_device *dpbp_dev;
>  	struct dpbp_attr dpbp_attrs;
> +	struct dpaa2_eth_bp *bp;
> +	int err;
>  
>  	err = fsl_mc_object_allocate(to_fsl_mc_device(dev), FSL_MC_POOL_DPBP,
>  				     &dpbp_dev);
> @@ -3219,12 +3244,16 @@ static int dpaa2_eth_setup_dpbp(struct dpaa2_eth_priv *priv)
>  			err = -EPROBE_DEFER;
>  		else
>  			dev_err(dev, "DPBP device allocation failed\n");
> -		return err;
> +		return ERR_PTR(err);
>  	}
>  
> -	priv->dpbp_dev = dpbp_dev;
> +	bp = kzalloc(sizeof(*bp), GFP_KERNEL);
> +	if (!bp) {
> +		err = -ENOMEM;
> +		goto err_alloc;
> +	}

It looks like 'bp' is leaked on later error paths.

Cheers,

Paolo

Powered by blists - more mailing lists