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

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]

Message-ID:
 <PAXPR04MB8510D3A2F2A792A89941A7FD88D22@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Fri, 14 Mar 2025 04:51:06 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: Claudiu Manoil <claudiu.manoil@....com>, Clark Wang
	<xiaoning.wang@....com>, "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "christophe.leroy@...roup.eu"
	<christophe.leroy@...roup.eu>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v4 net-next 02/14] net: enetc: add command BD ring support
 for i.MX95 ENETC

> 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@....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()

Yes, for PSI we can use directly call these functions because they are different
drivers, but for VSI, v1 and v4 will use the same driver. I just want the PSI and
VSI to be consistent. If you don't like this, I can remove these interfaces from
the patch set, and add vf_setup_cbdr and vf_teardown_cbdr in the future when
I add the VF support for ENETC v4.

> 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.