Re: [PATCH net-next v9 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:
- [Medium] Missing phy-mode property now unconditionally causes probe failure, breaking old device trees.

Pre-existing issues:
- [High] The spinlock `port->lock` is acquired using `spin_lock()` in process contexts without disabling softirqs, creating a deadlock risk if interrupted by a softirq calling `ndo_get_stats64`.
--

--- Patch [12]: [PATCH net-next v9 12/12] net: airoha: add phylink support ---
commit bb8a7d4b36e6cc8dd02385bcf6f62f3da412aa5a
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 59001fd4b6f74..ed1ac032f3374 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -1837,7 +1838,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 using spin_lock() here without disabling
softirqs creates a deadlock risk.

This function is called from process context (like ethtool callbacks), but it
is also called by airoha_dev_get_stats64() which can run in softirq context.
If a softirq interrupts the process-context caller while it holds port->lock,
the softirq handler will attempt to acquire the same lock and spin forever.

Should all process-context acquisitions of this lock be updated to use
spin_lock_bh() instead?

[ ... ]
> +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)
> +{
> +	struct airoha_gdm_dev *dev = container_of(config, struct airoha_gdm_dev,
> +						  phylink_config);
> +	struct airoha_gdm_port *port = dev->port;
> +	struct airoha_eth *eth = dev->eth;
> +	u32 frag_size_tx, frag_size_rx;
> +	u32 mask, val;
> +
> +	/* TX/RX frag is configured only for GDM4 */
> +	if (port->id != AIROHA_GDM4_IDX)
> +		return;
[ ... ]
> +	spin_lock(&port->lock);
> +
> +	/* Configure TX/RX frag based on speed */

[Severity: High]
This is a pre-existing issue with the locking design, but since
airoha_mac_link_up() is a new process-context caller, using spin_lock() here
introduces a new instance of the deadlock surface mentioned above against
softirq callers like airoha_dev_get_stats64().

Should this be changed to spin_lock_bh() as well?

[ ... ]
> +static int airoha_setup_phylink(struct net_device *netdev)
> +{
> +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	struct device_node *np = netdev->dev.of_node;
> +	struct airoha_gdm_port *port = dev->port;
> +	struct phylink_config *config;
> +	phy_interface_t phy_mode;
> +	struct phylink *phylink;
> +	int err;
> +
> +	err = of_get_phy_mode(np, &phy_mode);
> +	if (err) {
> +		dev_err(&netdev->dev, "incorrect phy-mode\n");
> +		return err;
> +	}

[Severity: Medium]
Does this mandatory check break backwards compatibility with older device trees?

If a device tree is missing the phy-mode property, this will now unconditionally
fail the driver probe, whereas the previous driver version allowed successful
probe without it.

Is there a fallback mode we should apply here instead of returning the error?

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