Re: [net-next PATCH 2/2] net: airoha: add phylink support for GDM1

Christian Marangi
2025-10-22T15:52:19Z

On Wed, Oct 22, 2025 at 09:42:11AM +0200, Maxime Chevallier wrote:
> Hi Christian,
>
> On 21/10/2025 21:33, Christian Marangi wrote:
> > In preparation for support of GDM2+ port, fill in phylink OPs for GDM1
> > that is an INTERNAL port for the Embedded Switch.
> >
> > Rework the GDM init logic by first preparing the struct with all the
> > required info and creating the phylink interface and only after the
> > parsing register the related netdev.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > drivers/net/ethernet/airoha/airoha_eth.c | 108 ++++++++++++++++++++---
> > drivers/net/ethernet/airoha/airoha_eth.h | 3 +
> > 2 files changed, 99 insertions(+), 12 deletions(-)
>
> You also need to select PHYLINK in Kconfig
>
> >
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index ce6d13b10e27..fc237775a998 100644
> > --- a/drivers/net/ethernet/airoha/airoha_eth.c
> > +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> > @@ -1613,6 +1613,8 @@ static int airoha_dev_open(struct net_device *dev)
> > struct airoha_gdm_port *port = netdev_priv(dev);
> > struct airoha_qdma *qdma = port->qdma;
> >
> > + phylink_start(port->phylink);
> > +
> > netif_tx_start_all_queues(dev);
> > err = airoha_set_vip_for_gdm_port(port, true);
> > if (err)
> > @@ -1665,6 +1667,8 @@ static int airoha_dev_stop(struct net_device *dev)
> > }
> > }
> >
> > + phylink_stop(port->phylink);
> > +
> > return 0;
> > }
> >
> > @@ -2813,6 +2817,17 @@ static const struct ethtool_ops airoha_ethtool_ops = {
> > .get_link = ethtool_op_get_link,
> > };
> >
> > +static struct phylink_pcs *airoha_phylink_mac_select_pcs(struct phylink_config *config,
> > + phy_interface_t interface)
> > +{
> > + return NULL;
> > +}
> > +
> > +static void airoha_mac_config(struct phylink_config *config, unsigned int mode,
> > + const struct phylink_link_state *state)
> > +{
> > +}
> > +
> > static int airoha_metadata_dst_alloc(struct airoha_gdm_port *port)
> > {
> > int i;
> > @@ -2857,6 +2872,55 @@ bool airoha_is_valid_gdm_port(struct airoha_eth *eth,
> > return false;
> > }
> >
> > +static void airoha_mac_link_up(struct phylink_config *config, struct phy_device *phy,
> > + unsigned int mode, phy_interface_t interface,
> > + int speed, int duplex, bool tx_pause, bool rx_pause)
> > +{
> > +}
> > +
> > +static void airoha_mac_link_down(struct phylink_config *config, unsigned int mode,
> > + phy_interface_t interface)
> > +{
> > +}
> > +
> > +static const struct phylink_mac_ops airoha_phylink_ops = {
> > + .mac_select_pcs = airoha_phylink_mac_select_pcs,
> > + .mac_config = airoha_mac_config,
> > + .mac_link_up = airoha_mac_link_up,
> > + .mac_link_down = airoha_mac_link_down,
> > +};
> > +
> > +static int airoha_setup_phylink(struct net_device *netdev)
> > +{
> > + struct airoha_gdm_port *port = netdev_priv(netdev);
> > + struct device *dev = &netdev->dev;
> > + phy_interface_t phy_mode;
> > + struct phylink *phylink;
> > +
> > + phy_mode = device_get_phy_mode(dev);
> > + if (phy_mode < 0) {
> > + dev_err(dev, "incorrect phy-mode\n");
> > + return phy_mode;
> > + }
> > +
> > + port->phylink_config.dev = dev;
> > + port->phylink_config.type = PHYLINK_NETDEV;
> > + port->phylink_config.mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> > + MAC_10000FD;
> > +
> > + __set_bit(PHY_INTERFACE_MODE_INTERNAL,
> > + port->phylink_config.supported_interfaces);
> > +
> > + phylink = phylink_create(&port->phylink_config, dev_fwnode(dev),
> > + phy_mode, &airoha_phylink_ops);
> > + if (IS_ERR(phylink))
> > + return PTR_ERR(phylink);
> > +
> > + port->phylink = phylink;
> > +
> > + return 0;
> > +}
> > +
> > static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> > struct device_node *np, int index)
> > {
> > @@ -2931,19 +2995,30 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
> > port->id = id;
> > eth->ports[p] = port;
> >
> > - err = airoha_metadata_dst_alloc(port);
> > - if (err)
> > - return err;
> > + return airoha_metadata_dst_alloc(port);
> > +}
> >
> > - err = register_netdev(dev);
> > - if (err)
> > - goto free_metadata_dst;
> > +static int airoha_register_gdm_ports(struct airoha_eth *eth)
> > +{
> > + int i;
> >
> > - return 0;
> > + for (i = 0; i < ARRAY_SIZE(eth->ports); i++) {
> > + struct airoha_gdm_port *port = eth->ports[i];
> > + int err;
> >
> > -free_metadata_dst:
> > - airoha_metadata_dst_free(port);
> > - return err;
> > + if (!port)
> > + continue;
> > +
> > + err = airoha_setup_phylink(port->dev);
> > + if (err)
> > + return err;
> > +
> > + err = register_netdev(port->dev);
> > + if (err)
> > + return err;
>
> The cleanup for that path seems to only be done if
>
> port->dev->reg_state == NETREG_REGISTERED
>
> So if netdev registration fails here, you'll never destroy
> the phylink instance :(
>

Oh ok I was with the (wrong) assumption that register_netdev always set
the state to REGISTERED but checking the actual function it's totally
not the case.

Wonder if there is a better method to check it but I guess making the
requested changed from Lorenzo should indirectly handle this BUG.

>
>

--
Ansuel