Re: [PATCH v5 net-next 02/14] net: enetc: add command BD ring support for i.MX95 ENETC

[Vladimir Oltean]

On Fri, Apr 11, 2025 at 05:57:40PM +0800, Wei Fang wrote:
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c 
> b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> index 20bfdf7fb4b4..ecb571e5ea50 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_cbdr.c
> @@ -60,6 +60,45 @@ void enetc_teardown_cbdr(struct enetc_cbdr *cbdr)
>  }
>  EXPORT_SYMBOL_GPL(enetc_teardown_cbdr);
>  
> +int enetc4_setup_cbdr(struct enetc_si *si)
> +{
> +     struct ntmp_user *user = &si->ntmp_user;
> +     struct device *dev = &si->pdev->dev;
> +     struct enetc_hw *hw = &si->hw;
> +     struct netc_cbdr_regs regs;
> +
> +     user->cbdr_num = 1;
> +     user->cbdr_size = NETC_CBDR_BD_NUM;
> +     user->dev = dev;
> +     user->ring = devm_kcalloc(dev, user->cbdr_num,
> +                               sizeof(struct netc_cbdr), GFP_KERNEL);
> +     if (!user->ring)
> +             return -ENOMEM;
> +
> +     /* set CBDR cache attributes */
> +     enetc_wr(hw, ENETC_SICAR2,
> +              ENETC_SICAR_RD_COHERENT | ENETC_SICAR_WR_COHERENT);
> +
> +     regs.pir = hw->reg + ENETC_SICBDRPIR;
> +     regs.cir = hw->reg + ENETC_SICBDRCIR;
> +     regs.mr = hw->reg + ENETC_SICBDRMR;
> +     regs.bar0 = hw->reg + ENETC_SICBDRBAR0;
> +     regs.bar1 = hw->reg + ENETC_SICBDRBAR1;
> +     regs.lenr = hw->reg + ENETC_SICBDRLENR;
> +
> +     return netc_setup_cbdr(dev, user->cbdr_size, &regs, user->ring);
> +}
> +EXPORT_SYMBOL_GPL(enetc4_setup_cbdr);
> +
> +void enetc4_teardown_cbdr(struct enetc_si *si)
> +{
> +     struct ntmp_user *user = &si->ntmp_user;
> +
> +     netc_teardown_cbdr(user->dev, user->ring);
> +     user->dev = NULL;
> +}
> +EXPORT_SYMBOL_GPL(enetc4_teardown_cbdr);

I wanted to ask why isn't netc_setup_cbdr() merged into enetc4_setup_cbdr()
(and likewise for teardown_cbdr), because they sound very similar, and
they operate on the same data - one is literally a continuation of the
other. Then I looked downstream where the netc_switch is another API
user of netc_setup_cbdr() and netc_teardown_cbdr().

Do you think you could rename netc_setup_cbdr() into something like below:

struct ntmp_user *ntmp_user_create(struct device *dev, size_t num_cbdr,
                                   const struct netc_cbdr_regs *regs);
void ntmp_user_destroy(struct ntmp_user *user);

>From a data encapsulation perspective, it would be great if the outside
world only worked with an opaque struct ntmp_user * pointer.

Hide NETC_CBDR_BD_NUM from include/linux/fsl/ntmp.h if API users don't
need to customize it, and let ntmp_user_create() set it.

Move even more initialization into ntmp_user_create(), like the
allocation of "user->ring", and reduce the number of arguments.

In my opinion this would be a more natural organization of the code.