Message-ID: <20241024173451.wsdhghmz4vyboelu@skbuf>
Date: Thu, 24 Oct 2024 20:34:51 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Wei Fang <wei.fang@....com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, claudiu.manoil@....com, xiaoning.wang@....com,
	Frank.Li@....com, christophe.leroy@...roup.eu,
	linux@...linux.org.uk, bhelgaas@...gle.com, horms@...nel.org,
	imx@...ts.linux.dev, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, alexander.stein@...tq-group.com
Subject: Re: [PATCH v5 net-next 06/13] net: enetc: build enetc_pf_common.c as
 a separate module

On Thu, Oct 24, 2024 at 02:53:21PM +0800, Wei Fang wrote:
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_pf.h b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> index 92a26b09cf57..39db9d5c2e50 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_pf.h
> @@ -28,6 +28,16 @@ struct enetc_vf_state {
>  	enum enetc_vf_flags flags;
>  };
>  
> +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?

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

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

>  
>  	/* (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.

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

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?

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

Powered by blists - more mailing lists