Re: [PATCH net-next v2 3/3] net: mdio: support dynamic OF device changes

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The global OF overlay notifier inadvertently blocks the removal of any overlay containing non-MDIO nodes, and any overlay whose underlying MDIO bus is unregistered.
- [Low] Data race on `of_mdio_reconfig_owner` in the recursive lock implementation.
--

--- Patch [3]: [PATCH net-next v2 3/3] net: mdio: support dynamic OF device changes ---
commit c037ead180706f6895d9e354b94ae56ca7628104
Author: James Hilliard <[email protected]>

net: mdio: support dynamic OF device changes

This commit adds dynamic OF device support for MDIO buses. It enumerates PHYs
on OF reconfiguration addition events and handles removals by unregistering
devices attached to removed nodes. It also introduces serialization and overlay
preflight checks.

> diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c
> index 051e449bbe7c0..c89f88aacd8df 100644
> --- a/drivers/net/mdio/of_mdio.c
> +++ b/drivers/net/mdio/of_mdio.c
[ ... ]
> @@ -27,6 +30,52 @@ MODULE_AUTHOR("Grant Likely <[email protected]>");
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("OpenFirmware MDIO bus (Ethernet PHY) accessors");
>  
> +#if IS_ENABLED(CONFIG_OF_DYNAMIC)
> +/*
> + * OF changes can nest when probing one MDIO device enables another node on
> + * the same bus. Serialize independent changes while allowing that nesting.
> + */
> +static DEFINE_MUTEX(of_mdio_reconfig_mutex);
> +static struct task_struct *of_mdio_reconfig_owner;
> +static unsigned int of_mdio_reconfig_depth;
> +
> +static void of_mdio_reconfig_lock(void)
> +{
> +	if (!mutex_trylock(&of_mdio_reconfig_mutex)) {
> +		if (of_mdio_reconfig_owner == current) {

[Severity: Low]
Since `of_mdio_reconfig_owner` can be updated to NULL in
`of_mdio_reconfig_unlock()` concurrently, does this lockless read trigger a
KCSAN data race?

Would it be safer to use `READ_ONCE()` here and `WRITE_ONCE()` in the unlock
path to satisfy concurrency rules?

> +			of_mdio_reconfig_depth++;
> +			return;
> +		}
> +		mutex_lock(&of_mdio_reconfig_mutex);
> +	}
> +
> +	WARN_ON_ONCE(of_mdio_reconfig_owner);
> +	WARN_ON_ONCE(of_mdio_reconfig_depth);
> +	of_mdio_reconfig_owner = current;
> +	of_mdio_reconfig_depth = 1;
> +}
> +
> +static void of_mdio_reconfig_unlock(void)
> +{
> +	WARN_ON_ONCE(of_mdio_reconfig_owner != current);
> +	WARN_ON_ONCE(!of_mdio_reconfig_depth);
> +
> +	if (--of_mdio_reconfig_depth)
> +		return;
> +
> +	of_mdio_reconfig_owner = NULL;
> +	mutex_unlock(&of_mdio_reconfig_mutex);
> +}
[ ... ]
> +/* Overlay entry notifier errors cannot stop removal after the tree changed. */
> +static bool of_mdiobus_live_node_is_busy(struct device_node *node)
> +{
> +	struct device_node *scope;
> +	struct mii_bus *mdio;
> +	bool busy = true;
> +
> +	scope = of_mdiobus_get_removal_scope(node);
> +	mdio = of_mdiobus_find_parent(scope);
> +	if (!mdio)
> +		goto out_put_scope;

[Severity: High]
If a removed overlay contains non-MDIO nodes (for example, I2C devices),
`of_mdiobus_find_parent()` will return NULL.

Since `busy` is initialized to `true`, doesn't jumping to `out_put_scope`
return `true` in this case?

Because this overlay notifier is registered globally, does this inadvertently
block the removal of any overlay containing non-MDIO nodes system-wide by
falsely claiming the non-MDIO node is a busy MDIO device?

> +
> +	if (!mdiobus_device_change_begin(mdio, true)) {
> +		busy = of_mdiobus_node_is_busy(mdio, scope);
> +		mdiobus_device_change_end(mdio, true);
> +	}
> +	put_device(&mdio->dev);
> +
> +out_put_scope:
> +	of_node_put(scope);
> +	return busy;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3