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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <69033f5a.df0a0220.25fede.548f@mx.google.com>
Date: Thu, 30 Oct 2025 11:35:04 +0100
From: Christian Marangi <ansuelsmth@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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>,
	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 v2 2/2] net: airoha: add phylink support for GDM1

On Sat, Oct 25, 2025 at 09:36:19PM +0100, Russell King (Oracle) wrote:
> On Thu, Oct 23, 2025 at 04:58:49PM +0200, 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.
> > 
> > Add all the phylink start/stop and fill in the MAC capabilities and the
> > internal interface as the supported interface.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > ---
> >  drivers/net/ethernet/airoha/Kconfig      |  1 +
> >  drivers/net/ethernet/airoha/airoha_eth.c | 77 +++++++++++++++++++++++-
> >  drivers/net/ethernet/airoha/airoha_eth.h |  3 +
> >  3 files changed, 80 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/airoha/Kconfig b/drivers/net/ethernet/airoha/Kconfig
> > index ad3ce501e7a5..3c74438bc8a0 100644
> > --- a/drivers/net/ethernet/airoha/Kconfig
> > +++ b/drivers/net/ethernet/airoha/Kconfig
> > @@ -2,6 +2,7 @@
> >  config NET_VENDOR_AIROHA
> >  	bool "Airoha devices"
> >  	depends on ARCH_AIROHA || COMPILE_TEST
> > +	select PHYLIB
> 
> This looks wrong if you're using phylink.
> 
> >  	help
> >  	  If you have a Airoha SoC with ethernet, say Y.
> >  
> > diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> > index ce6d13b10e27..deba909104bb 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)
> 
> phylink_start() _can_ bring the carrier up immediately. Is the netdev
> ready to start operating at the point phylink_start() has been called?
> This error handling suggests the answer is "no", and the lack of
> phylink_stop() in the error path is also a red flag.
>

So I guess the correct way is to move start at the very end of dev_open.

> > @@ -1665,6 +1667,8 @@ static int airoha_dev_stop(struct net_device *dev)
> >  		}
> >  	}
> >  
> > +	phylink_stop(port->phylink);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2813,6 +2817,18 @@ 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)
> 
> I'd write this as:
> 
> static struct phylink_pcs *
> airoha_phylink_mac_select_pcs(struct phylink_config *config,
> 			      phy_interface_t interface)
> 
> but:
> 
> > +{
> > +	return NULL;
> > +}
> 
> Not sure what the point of this is, as this will be the effect if
> this function is not provided.
> 

Sorry I was confused with the other OPs that are mandatory or a kernel
panic is triggered if not defined. (for example the MAC config)

> > +
> > +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 +2873,57 @@ 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,
> > +};
> 
> All the called methods are entirely empty, meaning that anything that
> phylink reports may not reflect what is going on with the device.
> 
> Is there a plan to implement these methods?
> 

Yes. For the internal port there isn't much to configure but when the
PCS for the other GDM port will be implemented, those will be filled in.
This is really to implement the generic part and prevent having a
massive series later (as it will be already quite big and if not more
than 10-12 patch)

Hope it's understandable why all these empty functions.

-- 
	Ansuel