Re: [net-next, 06/10] bng_en: Add backing store support

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 24/06/2025 11:29, Vikas Gupta wrote:
> On Thu, Jun 19, 2025 at 6:32 PM Vadim Fedorenko
> <vadim.fedorenko@linux.dev> wrote:
>>
>> On 18/06/2025 15:47, Vikas Gupta wrote:
>>> Backing store or context memory on the host helps the
>>> device to manage rings, stats and other resources.
>>> Context memory is allocated with the help of ring
>>> alloc/free functions.
>>>
>>> Signed-off-by: Vikas Gupta <vikas.gupta@broadcom.com>
>>> Reviewed-by: Bhargava Chenna Marreddy <bhargava.marreddy@broadcom.com>
>>> Reviewed-by: Rajashekar Hudumula <rajashekar.hudumula@broadcom.com>
>>> ---
>>>    drivers/net/ethernet/broadcom/bnge/bnge.h     |  18 +
>>>    .../ethernet/broadcom/bnge/bnge_hwrm_lib.c    | 168 +++++++++
>>>    .../ethernet/broadcom/bnge/bnge_hwrm_lib.h    |   4 +
>>>    .../net/ethernet/broadcom/bnge/bnge_rmem.c    | 337 ++++++++++++++++++
>>>    .../net/ethernet/broadcom/bnge/bnge_rmem.h    | 153 ++++++++
>>>    5 files changed, 680 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge.h b/drivers/net/ethernet/broadcom/bnge/bnge.h
>>> index 60af0517c45e..01f64a10729c 100644
>>> --- a/drivers/net/ethernet/broadcom/bnge/bnge.h
>>> +++ b/drivers/net/ethernet/broadcom/bnge/bnge.h
>>> @@ -9,6 +9,7 @@
>>>
>>>    #include <linux/etherdevice.h>
>>>    #include "../bnxt/bnxt_hsi.h"
>>> +#include "bnge_rmem.h"
>>>
>>>    #define DRV_VER_MAJ 1
>>>    #define DRV_VER_MIN 15
>>> @@ -52,6 +53,13 @@ enum {
>>>        BNGE_FW_CAP_VNIC_RE_FLUSH                       = BIT_ULL(26),
>>>    };
>>>
>>> +enum {
>>> +     BNGE_EN_ROCE_V1                                 = BIT_ULL(0),
>>> +     BNGE_EN_ROCE_V2                                 = BIT_ULL(1),
>>> +};
>>> +
>>> +#define BNGE_EN_ROCE         (BNGE_EN_ROCE_V1 | BNGE_EN_ROCE_V2)
>>> +
>>>    struct bnge_dev {
>>>        struct device   *dev;
>>>        struct pci_dev  *pdev;
>>> @@ -89,6 +97,16 @@ struct bnge_dev {
>>>    #define BNGE_STATE_DRV_REGISTERED      0
>>>
>>>        u64                     fw_cap;
>>> +
>>> +     /* Backing stores */
>>> +     struct bnge_ctx_mem_info        *ctx;
>>> +
>>> +     u64                     flags;
>>>    };
>>>
>>> +static inline bool bnge_is_roce_en(struct bnge_dev *bd)
>>> +{
>>> +     return bd->flags & BNGE_EN_ROCE;
>>> +}
>>> +
>>>    #endif /* _BNGE_H_ */
>>> diff --git a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
>>> index 567376a407df..e5f32ac8a69f 100644
>>> --- a/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
>>> +++ b/drivers/net/ethernet/broadcom/bnge/bnge_hwrm_lib.c
>>> @@ -10,6 +10,7 @@
>>>    #include "../bnxt/bnxt_hsi.h"
>>>    #include "bnge_hwrm.h"
>>>    #include "bnge_hwrm_lib.h"
>>> +#include "bnge_rmem.h"
>>>
>>>    int bnge_hwrm_ver_get(struct bnge_dev *bd)
>>>    {
>>> @@ -211,3 +212,170 @@ int bnge_hwrm_func_drv_unrgtr(struct bnge_dev *bd)
>>>                return rc;
>>>        return hwrm_req_send(bd, req);
>>>    }
>>> +
>>> +static void bnge_init_ctx_initializer(struct bnge_ctx_mem_type *ctxm,
>>> +                                   u8 init_val, u8 init_offset,
>>> +                                   bool init_mask_set)
>>> +{
>>> +     ctxm->init_value = init_val;
>>> +     ctxm->init_offset = BNGE_CTX_INIT_INVALID_OFFSET;
>>> +     if (init_mask_set)
>>> +             ctxm->init_offset = init_offset * 4;
>>> +     else
>>> +             ctxm->init_value = 0;
>>> +}
>>> +
>>> +static int bnge_alloc_all_ctx_pg_info(struct bnge_dev *bd, int ctx_max)
>>> +{
>>> +     struct bnge_ctx_mem_info *ctx = bd->ctx;
>>> +     u16 type;
>>> +
>>> +     for (type = 0; type < ctx_max; type++) {
>>> +             struct bnge_ctx_mem_type *ctxm = &ctx->ctx_arr[type];
>>> +             int n = 1;
>>> +
>>> +             if (!ctxm->max_entries)
>>> +                     continue;
>>> +
>>> +             if (ctxm->instance_bmap)
>>> +                     n = hweight32(ctxm->instance_bmap);
>>> +             ctxm->pg_info = kcalloc(n, sizeof(*ctxm->pg_info), GFP_KERNEL);
>>> +             if (!ctxm->pg_info)
>>> +                     return -ENOMEM;
>>
>> It's a bit hard to be absolutely sure without full chain of calls, but
>> it looks like some of the memory can be leaked in case of allocation
>> fail. Direct callers do not clear allocated contextes in the error path.
> 
> After the allocation of context memory, it could be in use by the
> hardware, so it is always safer
> to clean it up when we finally deregister from the firmware. The
> bnge_fw_unregister_dev() function
> handles memory freeing via bnge_free_ctx_mem(), instead of freeing up by caller.
> I hope this clarifies your concern.

Yeah, I have found cleaning calls in the other patches, thanks

> 
>>
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +