Re: [PATCH v2 05/13] can: slcan: simplify the device de-allocation

[Oliver Hartkopp]

This patch (at least) needs some rework.

The patch cf124db566e6b036 ("net: Fix inconsistent teardown and release 
of private netdev state.") from DaveM added some priv_destructor

    dev->priv_destructor = sl_free_netdev;

which is not taken into account in this patch.

As written before I would like to discuss this change out of your patch 
series "can: slcan: extend supported features" as it is no slcan feature 
extension AND has to be synchronized with the drivers/net/slip/slip.c 
implementation.

When it has not real benefit and introduces more code and may create 
side effects, this beautification should probably be omitted at all.

Thanks,
Oliver

On 08.06.22 18:51, Dario Binacchi wrote:
> Since slcan_devs array contains the addresses of the created devices, I
> think it is more natural to use its address to remove it from the list.
> It is not necessary to store the index of the array that points to the
> device in the driver's private data.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> (no changes since v1)
>
>   drivers/net/can/slcan.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index 929cb55e08af..cf05c30b8da5 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -432,11 +432,17 @@ static int slc_open(struct net_device *dev)
>   
>   static void slc_dealloc(struct slcan *sl)
>   {
> -	int i = sl->dev->base_addr;
> +	unsigned int i;
>   
> -	free_candev(sl->dev);
> -	if (slcan_devs)
> -		slcan_devs[i] = NULL;
> +	for (i = 0; i < maxdev; i++) {
> +		if (sl->dev == slcan_devs[i]) {
> +			free_candev(sl->dev);
> +			slcan_devs[i] = NULL;
> +			return;
> +		}
> +	}
> +
> +	pr_err("slcan: can't free %s resources\n",  sl->dev->name);
>   }
>   
>   static int slcan_change_mtu(struct net_device *dev, int new_mtu)
> @@ -533,7 +539,6 @@ static struct slcan *slc_alloc(void)
>   
>   	snprintf(dev->name, sizeof(dev->name), "slcan%d", i);
>   	dev->netdev_ops = &slc_netdev_ops;
> -	dev->base_addr  = i;
>   	sl = netdev_priv(dev);
>   
>   	/* Initialize channel control data */