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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 07.06.22 11:47, 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.

IMO this patch should not be part of the slcan enhancement series.

I can see the "miss-use" of dev->base_addr but when we change this code 
we should also take care of a similar handling in drivers/net/slip/slip.c

Therefore a change like this should be done in slcan.c and slip.c 
simultaneously with a single patch.

Best regards,
Oliver

> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
> 
>   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 956b47bd40a7..4df0455e11a2 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -428,11 +428,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)
> @@ -529,7 +535,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 */
>