Message-ID: <68f8fda8.5d0a0220.3519eb.41e3@mx.google.com>
Date: Wed, 22 Oct 2025 17:52:06 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: Lorenzo Bianconi <lorenzo@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Russell King <linux@...linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH 2/2] net: airoha: add phylink support for GDM1

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

Powered by blists - more mailing lists