Re: [PATCH net-next v10 03/11] net: phylink: add phylink_release_pcs() to externally release a PCS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] phylink_release_pcs() unconditionally calls phylink_link_down(), leading to unbalanced and duplicate mac_link_down() calls if the link is already down.
- [Medium] The polling timer `pl->link_poll` is stopped using non-synchronous `timer_delete()`, allowing it to leak and endlessly poll if racing with the timer callback.
- [Low] Missing WRITE_ONCE() when updating pl->pcs to NULL, causing a data race with concurrent readers.
--

--- Patch [3]: [PATCH net-next v10 03/11] net: phylink: add phylink_release_pcs() to externally release a PCS ---
commit ccb6652ab082daee833a77e32118d4a3d55ac68e
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add phylink_release_pcs() to externally release a PCS

This commit adds phylink_release_pcs() to externally release a PCS from a
phylink instance. It removes the PCS from the internal list, reparses the
supported interfaces, and triggers a phylink resolve to handle the removal.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a5e2d342a92e1..562e09d1838b0 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -917,6 +920,63 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
>  	}
>  }
>  
> +/**
> + * phylink_release_pcs - Removes a PCS from the phylink PCS available list
> + * @pcs: a pointer to the phylink_pcs struct to be released
> + *
> + * This function release a PCS from the phylink PCS available list if
> + * actually in use. It also refreshes the supported interfaces of the
> + * phylink instance by copying the supported interfaces from the phylink
> + * conf and merging the supported interfaces of the remaining available PCS
> + * in the list and trigger a resolve.
> + */
> +void phylink_release_pcs(struct phylink_pcs *pcs)
> +{
[ ... ]
> +	/*
> +	 * Check if we are removing the PCS currently
> +	 * in use by phylink. If this is the case, tear down
> +	 * the link, force phylink resolve to reconfigure the
> +	 * interface mode, disable the current PCS and set the
> +	 * phylink PCS to NULL.
> +	 */
> +	if (pl->pcs == pcs) {
> +		phylink_link_down(pl);

[Severity: High]
Could this call phylink_link_down() when the link is already down?

If phylink_release_pcs() is called while the link is already administratively
or physically down, we might invoke mac_link_down() a second time. Other
phylink core functions protect against this by checking pl->old_link_state
before tearing down the link:

    if (pl->old_link_state) {
        phylink_link_down(pl);
        pl->old_link_state = false;
    }

Will calling mac_link_down() twice cause unbalanced reference counts or
disable clocks multiple times?

> +		pl->old_link_state = false;
> +		phylink_pcs_poll_stop(pl);

[Severity: Medium]
Does this leave the link_poll timer running if it races with the timer
callback?

Looking at phylink_pcs_poll_stop(), it stops the timer using a
non-synchronous timer_delete():

static void phylink_pcs_poll_stop(struct phylink *pl)
{
    if (pl->cfg_link_an_mode == MLO_AN_INBAND)
        timer_delete(&pl->link_poll);
}

If phylink_release_pcs() runs concurrently with the timer callback on
another CPU, timer_delete() returns immediately. The timer callback
unconditionally re-arms itself with mod_timer(), which would allow the
timer to run indefinitely after the PCS is removed.

Should this use timer_delete_sync() or otherwise ensure the timer is fully
stopped?

> +		phylink_pcs_disable(pl->pcs);
> +
> +		pl->force_major_config = true;
> +		pl->pcs = NULL;

[Severity: Low]
Should this update to pl->pcs use WRITE_ONCE()?

phylink_pcs_change() reads pl->pcs locklessly from asynchronous contexts:
        
    if (!pl || READ_ONCE(pl->pcs) != pcs)

Doing a plain store here could cause a data race with concurrent readers
and trigger KCSAN warnings.

> +	}
> +
> +	mutex_unlock(&pl->state_mutex);
[ ... ]

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