Message-ID:
 <PAXPR04MB8510468F88A236EA7ABB76D8884F2@PAXPR04MB8510.eurprd04.prod.outlook.com>
Date: Fri, 25 Oct 2024 03:00:42 +0000
From: Wei Fang <wei.fang@....com>
To: Vladimir Oltean <vladimir.oltean@....com>
CC: "davem@...emloft.net" <davem@...emloft.net>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "kuba@...nel.org" <kuba@...nel.org>,
	"pabeni@...hat.com" <pabeni@...hat.com>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, Claudiu Manoil <claudiu.manoil@....com>, Clark Wang
	<xiaoning.wang@....com>, Frank Li <frank.li@....com>,
	"christophe.leroy@...roup.eu" <christophe.leroy@...roup.eu>,
	"linux@...linux.org.uk" <linux@...linux.org.uk>, "bhelgaas@...gle.com"
	<bhelgaas@...gle.com>, "horms@...nel.org" <horms@...nel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-pci@...r.kernel.org"
	<linux-pci@...r.kernel.org>, "alexander.stein@...tq-group.com"
	<alexander.stein@...tq-group.com>
Subject: RE: [PATCH v5 net-next 06/13] net: enetc: build enetc_pf_common.c as
 a separate module

> > +struct enetc_pf;
> > +
> > +struct enetc_pf_ops {
> > +	void (*set_si_primary_mac)(struct enetc_hw *hw, int si, const u8 *addr);
> > +	void (*get_si_primary_mac)(struct enetc_hw *hw, int si, u8 *addr);
> > +	struct phylink_pcs *(*create_pcs)(struct enetc_pf *pf, struct mii_bus *bus);
> > +	void (*destroy_pcs)(struct phylink_pcs *pcs);
> > +	int (*enable_psfp)(struct enetc_ndev_priv *priv);
> > +};
> > +
> >  struct enetc_pf {
> >  	struct enetc_si *si;
> >  	int num_vfs; /* number of active VFs, after sriov_init */
> > @@ -50,6 +60,8 @@ struct enetc_pf {
> >
> >  	phy_interface_t if_mode;
> >  	struct phylink_config phylink_config;
> > +
> > +	const struct enetc_pf_ops *ops;
> >  };
> >
> >  #define phylink_to_enetc_pf(config) \
> > @@ -59,9 +71,6 @@ int enetc_msg_psi_init(struct enetc_pf *pf);
> >  void enetc_msg_psi_free(struct enetc_pf *pf);
> >  void enetc_msg_handle_rxmsg(struct enetc_pf *pf, int mbox_id, u16
> *status);
> >
> > -void enetc_pf_get_primary_mac_addr(struct enetc_hw *hw, int si, u8 *addr);
> > -void enetc_pf_set_primary_mac_addr(struct enetc_hw *hw, int si,
> > -				   const u8 *addr);
> >  int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr);
> >  int enetc_setup_mac_addresses(struct device_node *np, struct enetc_pf
> *pf);
> >  void enetc_pf_netdev_setup(struct enetc_si *si, struct net_device *ndev,
> > @@ -71,3 +80,9 @@ void enetc_mdiobus_destroy(struct enetc_pf *pf);
> >  int enetc_phylink_create(struct enetc_ndev_priv *priv, struct device_node
> *node,
> >  			 const struct phylink_mac_ops *ops);
> >  void enetc_phylink_destroy(struct enetc_ndev_priv *priv);
> > +
> > +static inline void enetc_pf_ops_register(struct enetc_pf *pf,
> > +					 const struct enetc_pf_ops *ops)
> > +{
> > +	pf->ops = ops;
> > +}
> 
> I think this is more confusing than helpful? "register" suggests there
> should also be an "unregister" coming. Either "set" or just open-code
> the assignment?

Okay, I can remove this helper function, just use open-code.

> 
> > diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > index bce81a4f6f88..94690ed92e3f 100644
> > --- a/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf_common.c
> > @@ -8,19 +8,37 @@
> >
> >  #include "enetc_pf.h"
> >
> > +static int enetc_set_si_hw_addr(struct enetc_pf *pf, int si, u8 *mac_addr)
> > +{
> > +	struct enetc_hw *hw = &pf->si->hw;
> > +
> > +	if (pf->ops->set_si_primary_mac)
> > +		pf->ops->set_si_primary_mac(hw, si, mac_addr);
> > +	else
> > +		return -EOPNOTSUPP;
> > +
> > +	return 0;
> 
> Don't artificially create errors when there are really no errors to handle.
> Both enetc_pf_ops and enetc4_pf_ops provide .set_si_primary_mac(), so it
> is unnecessary to handle the case where it isn't present. Those functions
> return void, and void can be propagated to enetc_set_si_hw_addr() as well.
> 

I thought checking the pointer is safer, so you mean that pointers that are
definitely present in the current driver do not need to be checked?

> > +}
> > +
> >  int enetc_pf_set_mac_addr(struct net_device *ndev, void *addr)
> >  {
> >  	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> > +	struct enetc_pf *pf = enetc_si_priv(priv->si);
> >  	struct sockaddr *saddr = addr;
> > +	int err;
> >
> >  	if (!is_valid_ether_addr(saddr->sa_data))
> >  		return -EADDRNOTAVAIL;
> >
> > +	err = enetc_set_si_hw_addr(pf, 0, saddr->sa_data);
> > +	if (err)
> > +		return err;
> > +
> >  	eth_hw_addr_set(ndev, saddr->sa_data);
> > -	enetc_pf_set_primary_mac_addr(&priv->si->hw, 0, saddr->sa_data);
> 
> This isn't one for one replacement, it also moves the function call
> relative to eth_hw_addr_set() without making any mention about that
> movement being safe. And even if safe, it is logically a separate change
> which deserves its own merit analysis in another patch (if there's no
> merit, leave it where it is).
> 
> I believe the merit was: enetc_set_si_hw_addr() can return error, thus
> we simplify the control flow if we call it prior to eth_hw_addr_set() -
> nothing to unroll. But as explained above, enetc_set_si_hw_addr() cannot
> actually fail, so there is no real merit.
> 
> >
> >  	return 0;
> >  }
> > +EXPORT_SYMBOL_GPL(enetc_pf_set_mac_addr);
> >
> >  static int enetc_setup_mac_address(struct device_node *np, struct
> enetc_pf *pf,
> >  				   int si)
> > @@ -38,8 +56,8 @@ static int enetc_setup_mac_address(struct device_node
> *np, struct enetc_pf *pf,
> >  	}
> >
> >  	/* (2) bootloader supplied MAC address */
> > -	if (is_zero_ether_addr(mac_addr))
> > -		enetc_pf_get_primary_mac_addr(hw, si, mac_addr);
> > +	if (is_zero_ether_addr(mac_addr) && pf->ops->get_si_primary_mac)
> > +		pf->ops->get_si_primary_mac(hw, si, mac_addr);
> 
> Again, if there's no reason for the method to be optional (both PF
> drivers support it), don't make it optional.
> 
> A bit inconsistent that pf->ops->set_si_primary_mac() goes through a
> wrapper function but this doesn't.
> 

If we really do not need to check these callback pointers, then I think I can
remove the wrapper.

> >
> >  	/* (3) choose a random one */
> >  	if (is_zero_ether_addr(mac_addr)) {
> > @@ -48,7 +66,9 @@ static int enetc_setup_mac_address(struct device_node
> *np, struct enetc_pf *pf,
> >  			 si, mac_addr);
> >  	}
> >
> > -	enetc_pf_set_primary_mac_addr(hw, si, mac_addr);
> > +	err = enetc_set_si_hw_addr(pf, si, mac_addr);
> > +	if (err)
> > +		return err;
> 
> This should go back to normal (no "err = ...; if (err) return err") once
> the function is made void.
> 
> >
> >  	return 0;
> >  }
> > @@ -107,7 +129,8 @@ void enetc_pf_netdev_setup(struct enetc_si *si,
> struct net_device *ndev,
> >  			     NETDEV_XDP_ACT_NDO_XMIT |
> NETDEV_XDP_ACT_RX_SG |
> >  			     NETDEV_XDP_ACT_NDO_XMIT_SG;
> >
> > -	if (si->hw_features & ENETC_SI_F_PSFP && !enetc_psfp_enable(priv)) {
> > +	if (si->hw_features & ENETC_SI_F_PSFP && pf->ops->enable_psfp &&
> > +	    !pf->ops->enable_psfp(priv)) {
> 
> This one looks extremely weird in the existing code, but I suppose I'm
> too late to the party to request you to clean up any of the PSFP code,
> so I'll make a note to do it myself after your work. I haven't spotted
> any actual bug, just weird coding patterns.
> 
> No change request here. I see the netc4_pf doesn't implement enable_psfp(),
> so making it optional here is fine.

Yes, PSFP is not supported in this patch set, I will remove it in future.

> 
> >  		priv->active_offloads |= ENETC_F_QCI;
> >  		ndev->features |= NETIF_F_HW_TC;
> >  		ndev->hw_features |= NETIF_F_HW_TC;
> > @@ -116,6 +139,7 @@ void enetc_pf_netdev_setup(struct enetc_si *si,
> struct net_device *ndev,
> >  	/* pick up primary MAC address from SI */
> >  	enetc_load_primary_mac_addr(&si->hw, ndev);
> >  }
> > +EXPORT_SYMBOL_GPL(enetc_pf_netdev_setup);
> >
> >  static int enetc_mdio_probe(struct enetc_pf *pf, struct device_node *np)
> >  {
> > @@ -162,6 +186,9 @@ static int enetc_imdio_create(struct enetc_pf *pf)
> >  	struct mii_bus *bus;
> >  	int err;
> >
> > +	if (!pf->ops->create_pcs)
> > +		return -EOPNOTSUPP;
> > +
> 
> I don't understand how this works. You don't implement create_pcs() for
> netc4_pf until the very end of the series. Probing will fail for SerDes
> interfaces (enetc_port_has_pcs() returns true) and that's fine?
> 

Currently, we have not add the PCS support, so the 10G ENETC is not supported
yet. And we also disable the 10G ENETC in DTS. Only the 1G ENETCs (without PCS)
are supported for i.MX95.

> A message maybe, stating what's the deal? Just that users figure out
> quickly that it's an expected behavior, and not spend hours debugging
> until they find out it's not their fault?

I will explain in the commit message that i.MX95 10G ENETC is not currently
supported.

> 
> >  	bus = mdiobus_alloc_size(sizeof(*mdio_priv));
> >  	if (!bus)
> >  		return -ENOMEM;

Powered by blists - more mailing lists