Re: [PATCH net-next v5 12/14] net: renesas: rswitch: add passing of rswitch_private into notifiers

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: renesas: rswitch: add passing of rswitch_private into notifiers

This commit provides the rswitch_private structure to the network notifiers.
This allows the notification handlers to access the hardware state and
device-specific data.

> diff --git a/drivers/net/ethernet/renesas/rswitch_l2.c b/drivers/net/ethernet/renesas/rswitch_l2.c
> index c76fc8121ef05..d2deb7cb76349 100644
> --- a/drivers/net/ethernet/renesas/rswitch_l2.c
> +++ b/drivers/net/ethernet/renesas/rswitch_l2.c

[ ... ]

> -int rswitch_register_notifiers(void)
> +int rswitch_register_notifiers(struct rswitch_private *priv)
>  {
> -	int ret;
> +	int err;
>  
> -	ret = register_netdevice_notifier(&rswitch_netdevice_nb);
> -	if (ret)
> +	priv->rswitch_netdevice_nb.notifier_call = rswitch_netdevice_event;
> +	err = register_netdevice_notifier(&priv->rswitch_netdevice_nb);

[Severity: High]
Does moving the notifier blocks into the per-device structure without
filtering by instance cause duplicate event processing?

Since callbacks like rswitch_netdevice_event only check if the device is an
rswitch netdevice, rather than verifying it belongs to this specific priv
instance, it seems every registered callback will process the same event if
multiple driver instances exist. Could this lead to events on one instance
inadvertently modifying the hardware registers of another?

> +	if (err)
>  		goto register_netdevice_notifier_failed;

[ ... ]

> diff --git a/drivers/net/ethernet/renesas/rswitch_main.c b/drivers/net/ethernet/renesas/rswitch_main.c
> index da400020a8f19..cfe7bfe6546b2 100644
> --- a/drivers/net/ethernet/renesas/rswitch_main.c
> +++ b/drivers/net/ethernet/renesas/rswitch_main.c
> @@ -2241,6 +2241,30 @@ static int rswitch_init(struct rswitch_private *priv)
>  	return err;
>  }
>  
> +static void rswitch_deinit(struct rswitch_private *priv)
> +{
> +	unsigned int i;
> +
> +	rswitch_gwca_hw_deinit(priv);
> +	rcar_gen4_ptp_unregister(priv->ptp_priv);
> +
> +	rswitch_for_each_enabled_port(priv, i) {
> +		struct rswitch_device *rdev = priv->rdev[i];
> +
> +		unregister_netdev(rdev->ndev);

[Severity: High]
This is a pre-existing issue, but does the ordering in this teardown path
cause a use-after-free?

The hardware is reset and DMA queues are freed via rswitch_gwca_hw_deinit()
before the loop that calls unregister_netdev(). If NAPI is actively polling
or user space transmits packets while the queues are being freed, could this
crash? Should unregister_netdev() be called to synchronously stop queues
before destroying the hardware state?

> +		rswitch_ether_port_deinit_one(rdev);
> +		phy_exit(priv->rdev[i]->serdes);
> +	}

[ ... ]

> @@ -2317,9 +2341,13 @@ static int renesas_eth_sw_probe(struct platform_device *pdev)
>  	if (list_empty(&priv->port_list))
>  		dev_warn(&pdev->dev, "could not initialize any ports\n");
>  
> -	ret = rswitch_register_notifiers();
> +	ret = rswitch_register_notifiers(priv);
>  	if (ret) {
> +		rswitch_deinit(priv);
> +		pm_runtime_put(&pdev->dev);
> +		pm_runtime_disable(&pdev->dev);

[Severity: Medium]
Can this asynchronous put leave the hardware in an active power state?

The call to pm_runtime_put() queues the idle work asynchronously, but
pm_runtime_disable() includes a barrier that might cancel the pending
suspend work before it executes. Should this use pm_runtime_put_sync()
instead to ensure the suspend completes before the device is disabled?

>  		dev_err(&pdev->dev, "could not register notifiers\n");
> +
>  		return ret;
>  	}