Messages in this thread | | | Subject | Re: [PATCH v2 05/13] can: slcan: simplify the device de-allocation | From | Oliver Hartkopp <> | Date | Wed, 8 Jun 2022 22:38:50 +0200 |
| |
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@amarulasolutions.com> > --- > > (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 */ >
| |