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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Tue, Mar 11, 2025 at 01:38:18PM +0800, Wei Fang wrote:
> The command BD ring is used to configure functionality where the
> underlying resources may be shared between different entities or being
> too large to configure using direct registers (such as lookup tables).
> 
> Because the command BD and table formats of i.MX95 and LS1028A are very
> different, the software processing logic is also different. In order to
> ensure driver compatibility, struct enetc_si_ops is introduced. This
> structure defines some hooks shared by VSI and PSI. Different hardware
> driver will register different hooks, For example, setup_cbdr() is used
> to initialize the command BD ring, and teardown_cbdr() is used to free
> the command BD ring.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.h  | 27 +++++++--
>  .../net/ethernet/freescale/enetc/enetc4_pf.c  | 47 +++++++++++++++-
>  .../net/ethernet/freescale/enetc/enetc_cbdr.c | 55 +++++++++++++++++--
>  .../net/ethernet/freescale/enetc/enetc_pf.c   | 13 +++--
>  .../net/ethernet/freescale/enetc/enetc_vf.c   | 13 +++--
>  5 files changed, 136 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index 4ad4eb5c5a74..4ff0957e69be 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.h
> @@ -8,6 +8,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/skbuff.h>
>  #include <linux/ethtool.h>
> +#include <linux/fsl/ntmp.h>
>  #include <linux/if_vlan.h>
>  #include <linux/phylink.h>
>  #include <linux/dim.h>
> @@ -266,6 +267,19 @@ struct enetc_platform_info {
>  	const struct enetc_drvdata *data;
>  };
>  
> +struct enetc_si;
> +
> +/*
> + * This structure defines the some common hooks for ENETC PSI and VSI.
> + * In addition, since VSI only uses the struct enetc_si as its private
> + * driver data, so this structure also define some hooks specifically
> + * for VSI. For VSI-specific hooks, the format is ‘vf_*()’.
> + */
> +struct enetc_si_ops {
> +	int (*setup_cbdr)(struct enetc_si *si);
> +	void (*teardown_cbdr)(struct enetc_si *si);
> +};

I don't understand the need for si->ops->setup_cbdr() and si->ops->teardown_cbdr()?
Doesn't every call site know which kind of SI it is dealing with, and thus it can
appropriately call the direct symbol?
- the v1 PSI and the VSI call enetc_setup_cbdr() and enetc_teardown_cbdr()
- the v4 PSI calls enetc4_setup_cbdr() and enetc4_teardown_cbdr()
What benefit is there to making an indirect function call?

At least that's what the current code does, I'm not sure if that is the intention.