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;
