| 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
| ||
|
Message-ID: <CAOBf=mvEEHeLP4CF76b7ip2VHz82V+c23trCkVeArjP9iJ0sfg@mail.gmail.com>
Date: Tue, 26 Nov 2024 12:00:54 +0530
From: Somnath Kotur <somnath.kotur@...adcom.com>
To: David Wei <dw@...idwei.uk>
Cc: netdev@...r.kernel.org, Michael Chan <michael.chan@...adcom.com>,
Andy Gospodarek <andrew.gospodarek@...adcom.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v1 1/3] bnxt_en: refactor tpa_info alloc/free into helpers
On Mon, Nov 25, 2024 at 9:54 AM David Wei <dw@...idwei.uk> wrote:
>
> Refactor bnxt_rx_ring_info->tpa_info operations into helpers that work
> on a single tpa_info in prep for queue API using them.
>
> There are 2 pairs of operations:
>
> * bnxt_alloc_one_tpa_info()
> * bnxt_free_one_tpa_info()
>
> These alloc/free the tpa_info array itself.
>
> * bnxt_alloc_one_tpa_info_data()
> * bnxt_free_one_tpa_info_data()
>
> These alloc/free the frags stored in tpa_info array.
>
> Signed-off-by: David Wei <dw@...idwei.uk>
> ---
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 148 ++++++++++++++--------
> 1 file changed, 95 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 5f7bdafcf05d..b2cc8df22241 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -3421,15 +3421,11 @@ static void bnxt_free_one_rx_agg_ring(struct bnxt *bp, struct bnxt_rx_ring_info
> }
> }
>
> -static void bnxt_free_one_rx_ring_skbs(struct bnxt *bp, int ring_nr)
> +static void bnxt_free_one_tpa_info_data(struct bnxt *bp,
> + struct bnxt_rx_ring_info *rxr)
> {
> - struct bnxt_rx_ring_info *rxr = &bp->rx_ring[ring_nr];
> - struct bnxt_tpa_idx_map *map;
> int i;
>
> - if (!rxr->rx_tpa)
> - goto skip_rx_tpa_free;
> -
> for (i = 0; i < bp->max_tpa; i++) {
> struct bnxt_tpa_info *tpa_info = &rxr->rx_tpa[i];
> u8 *data = tpa_info->data;
> @@ -3440,6 +3436,17 @@ static void bnxt_free_one_rx_ring_skbs(struct bnxt *bp, int ring_nr)
> tpa_info->data = NULL;
> page_pool_free_va(rxr->head_pool, data, false);
> }
> +}
> +
> +static void bnxt_free_one_rx_ring_skbs(struct bnxt *bp,
> + struct bnxt_rx_ring_info *rxr)
> +{
> + struct bnxt_tpa_idx_map *map;
> +
> + if (!rxr->rx_tpa)
> + goto skip_rx_tpa_free;
> +
> + bnxt_free_one_tpa_info_data(bp, rxr);
>
> skip_rx_tpa_free:
> if (!rxr->rx_buf_ring)
> @@ -3461,13 +3468,17 @@ static void bnxt_free_one_rx_ring_skbs(struct bnxt *bp, int ring_nr)
>
> static void bnxt_free_rx_skbs(struct bnxt *bp)
> {
> + struct bnxt_rx_ring_info *rxr;
> int i;
>
> if (!bp->rx_ring)
> return;
>
> - for (i = 0; i < bp->rx_nr_rings; i++)
> - bnxt_free_one_rx_ring_skbs(bp, i);
> + for (i = 0; i < bp->rx_nr_rings; i++) {
> + rxr = &bp->rx_ring[i];
> +
> + bnxt_free_one_rx_ring_skbs(bp, rxr);
Minor nit; Could avoid a declaration and an assignment here by
directly calling this API with the 2nd param set to &bp->rx_ring[i] ?
> + }
> }
>
> static void bnxt_free_skbs(struct bnxt *bp)
> @@ -3608,29 +3619,64 @@ static int bnxt_alloc_ring(struct bnxt *bp, struct bnxt_ring_mem_info *rmem)
> return 0;
> }
>
> +static void bnxt_free_one_tpa_info(struct bnxt *bp,
> + struct bnxt_rx_ring_info *rxr)
> +{
> + int i;
> +
> + kfree(rxr->rx_tpa_idx_map);
> + rxr->rx_tpa_idx_map = NULL;
> + if (rxr->rx_tpa) {
> + for (i = 0; i < bp->max_tpa; i++) {
> + kfree(rxr->rx_tpa[i].agg_arr);
> + rxr->rx_tpa[i].agg_arr = NULL;
> + }
> + }
> + kfree(rxr->rx_tpa);
> + rxr->rx_tpa = NULL;
> +}
> +
> static void bnxt_free_tpa_info(struct bnxt *bp)
> {
> - int i, j;
> + int i;
>
> for (i = 0; i < bp->rx_nr_rings; i++) {
> struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
>
> - kfree(rxr->rx_tpa_idx_map);
> - rxr->rx_tpa_idx_map = NULL;
> - if (rxr->rx_tpa) {
> - for (j = 0; j < bp->max_tpa; j++) {
> - kfree(rxr->rx_tpa[j].agg_arr);
> - rxr->rx_tpa[j].agg_arr = NULL;
> - }
> - }
> - kfree(rxr->rx_tpa);
> - rxr->rx_tpa = NULL;
> + bnxt_free_one_tpa_info(bp, rxr);
> + }
> +}
> +
> +static int bnxt_alloc_one_tpa_info(struct bnxt *bp,
> + struct bnxt_rx_ring_info *rxr)
> +{
> + struct rx_agg_cmp *agg;
> + int i;
> +
> + rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
> + GFP_KERNEL);
> + if (!rxr->rx_tpa)
> + return -ENOMEM;
> +
> + if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
> + return 0;
> + for (i = 0; i < bp->max_tpa; i++) {
> + agg = kcalloc(MAX_SKB_FRAGS, sizeof(*agg), GFP_KERNEL);
> + if (!agg)
> + return -ENOMEM;
> + rxr->rx_tpa[i].agg_arr = agg;
> }
> + rxr->rx_tpa_idx_map = kzalloc(sizeof(*rxr->rx_tpa_idx_map),
> + GFP_KERNEL);
> + if (!rxr->rx_tpa_idx_map)
> + return -ENOMEM;
> +
> + return 0;
> }
>
> static int bnxt_alloc_tpa_info(struct bnxt *bp)
> {
> - int i, j;
> + int i, rc;
>
> bp->max_tpa = MAX_TPA;
> if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
> @@ -3641,25 +3687,10 @@ static int bnxt_alloc_tpa_info(struct bnxt *bp)
>
> for (i = 0; i < bp->rx_nr_rings; i++) {
> struct bnxt_rx_ring_info *rxr = &bp->rx_ring[i];
> - struct rx_agg_cmp *agg;
>
> - rxr->rx_tpa = kcalloc(bp->max_tpa, sizeof(struct bnxt_tpa_info),
> - GFP_KERNEL);
> - if (!rxr->rx_tpa)
> - return -ENOMEM;
> -
> - if (!(bp->flags & BNXT_FLAG_CHIP_P5_PLUS))
> - continue;
> - for (j = 0; j < bp->max_tpa; j++) {
> - agg = kcalloc(MAX_SKB_FRAGS, sizeof(*agg), GFP_KERNEL);
> - if (!agg)
> - return -ENOMEM;
> - rxr->rx_tpa[j].agg_arr = agg;
> - }
> - rxr->rx_tpa_idx_map = kzalloc(sizeof(*rxr->rx_tpa_idx_map),
> - GFP_KERNEL);
> - if (!rxr->rx_tpa_idx_map)
> - return -ENOMEM;
> + rc = bnxt_alloc_one_tpa_info(bp, rxr);
> + if (rc)
> + return rc;
> }
> return 0;
> }
> @@ -4268,10 +4299,31 @@ static void bnxt_alloc_one_rx_ring_page(struct bnxt *bp,
> rxr->rx_agg_prod = prod;
> }
>
> +static int bnxt_alloc_one_tpa_info_data(struct bnxt *bp,
> + struct bnxt_rx_ring_info *rxr)
> +{
> + dma_addr_t mapping;
> + u8 *data;
> + int i;
> +
> + for (i = 0; i < bp->max_tpa; i++) {
> + data = __bnxt_alloc_rx_frag(bp, &mapping, rxr,
> + GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + rxr->rx_tpa[i].data = data;
> + rxr->rx_tpa[i].data_ptr = data + bp->rx_offset;
> + rxr->rx_tpa[i].mapping = mapping;
> + }
> +
> + return 0;
> +}
> +
> static int bnxt_alloc_one_rx_ring(struct bnxt *bp, int ring_nr)
> {
> struct bnxt_rx_ring_info *rxr = &bp->rx_ring[ring_nr];
> - int i;
> + int rc;
>
> bnxt_alloc_one_rx_ring_skb(bp, rxr, ring_nr);
>
> @@ -4281,19 +4333,9 @@ static int bnxt_alloc_one_rx_ring(struct bnxt *bp, int ring_nr)
> bnxt_alloc_one_rx_ring_page(bp, rxr, ring_nr);
>
> if (rxr->rx_tpa) {
> - dma_addr_t mapping;
> - u8 *data;
> -
> - for (i = 0; i < bp->max_tpa; i++) {
> - data = __bnxt_alloc_rx_frag(bp, &mapping, rxr,
> - GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> - rxr->rx_tpa[i].data = data;
> - rxr->rx_tpa[i].data_ptr = data + bp->rx_offset;
> - rxr->rx_tpa[i].mapping = mapping;
> - }
> + rc = bnxt_alloc_one_tpa_info_data(bp, rxr);
> + if (rc)
> + return rc;
> }
> return 0;
> }
> @@ -13657,7 +13699,7 @@ static void bnxt_rx_ring_reset(struct bnxt *bp)
> bnxt_reset_task(bp, true);
> break;
> }
> - bnxt_free_one_rx_ring_skbs(bp, i);
> + bnxt_free_one_rx_ring_skbs(bp, rxr);
> rxr->rx_prod = 0;
> rxr->rx_agg_prod = 0;
> rxr->rx_sw_agg_prod = 0;
> --
> 2.43.5
>
>
Other than the one minor suggestion, LGTM otherwise
Reviewed-by: Somnath Kotur <somnath.kotur@...adcom.com>
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4212 bytes)
Powered by blists - more mailing lists