Re: [RFC PATCH net-next v8 12/12] net: airoha: add phylink support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] PCS configuration fails because netdev->dev.fwnode is not initialized before calling fwnode_phylink_pcs_count() and fwnode_phylink_pcs_parse().

Pre-existing issues:
- [High] Spinlock deadlock due to acquiring `port->lock` without disabling softirqs.
--

commit c3dd483dacef136d470df9049b76a9b65a5390bc
Author: Christian Marangi <ansuelsmth@gmail.com>

net: airoha: add phylink support

Add phylink support for each GDM port. For GDM1 add the internal interface
mode as the only supported mode. For GDM2/3/4 add the required
configuration of the PCS to make the external PHY or attached SFP cage
work.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 64dde6464f3fb..3ea8deee3e950 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1788,7 +1789,7 @@ static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
>  	struct airoha_gdm_port *port = dev->port;
>  	int i;
>  
> -	spin_lock(&port->stats_lock);
> +	spin_lock(&port->lock);

[Severity: High]
This is a pre-existing issue, but can this cause a spinlock deadlock if a
softirq interrupts a process-context thread holding this lock?

This function is reachable from ndo_get_stats64, which the networking core
can invoke from softirq/BH context (e.g., via bonding or team drivers). If
this lock is also acquired in process context without disabling softirqs, a
deadlock can occur.

[ ... ]
> @@ -3168,6 +3199,155 @@ bool airoha_is_valid_gdm_dev(struct airoha_eth *eth,
[ ... ]
> +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)
> +{
[ ... ]
> +	spin_lock(&port->lock);

[Severity: High]
Does acquiring port->lock here expose the deadlock mentioned above?

airoha_mac_link_up() executes in process context. If a softirq interrupts it
while holding port->lock, the softirq might call airoha_update_hw_stats()
and attempt to acquire the exact same lock.

Should this and other process-context acquisitions of port->lock use
spin_lock_bh() instead?

> +	/* Configure TX/RX frag based on speed */
[ ... ]
> +static int airoha_setup_phylink(struct net_device *netdev)
> +{
[ ... ]
> +	} else {
> +		config->mac_capabilities = MAC_ASYM_PAUSE | MAC_SYM_PAUSE |
> +					   MAC_10 | MAC_100 | MAC_1000 |
> +					   MAC_2500FD | MAC_5000FD | MAC_10000FD;
> +
> +		config->num_possible_pcs = fwnode_phylink_pcs_count(dev_fwnode(config->dev));

[Severity: High]
Will dev_fwnode() return NULL here because netdev->dev.fwnode is not yet
initialized?

In airoha_alloc_gdm_device(), the device tree node is assigned using
netdev->dev.of_node = of_node_get(np), but the generic fwnode pointer is
left uninitialized.

Since airoha_setup_phylink() runs before register_netdevice(), dev_fwnode()
evaluates to NULL. This causes fwnode_phylink_pcs_count() to unconditionally
return 0, and fwnode_phylink_pcs_parse() to fail later in
airoha_fill_available_pcs().

Does this silently disable PCS discovery and break support for external PHYs
and SFP cages?

Could this be resolved by using device_set_node(&netdev->dev,
of_fwnode_handle(np)) in airoha_alloc_gdm_device(), or by passing
of_fwnode_handle(np) directly?

> +		config->fill_available_pcs = airoha_fill_available_pcs;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618125752.1223-1-ansuelsmth@gmail.com?part=12