| Message ID | 20220608165116.1575390-6-dario.binacchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 */
>
Hi, On Wed, Jun 8, 2022 at 10:38 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote: > > This patch (at least) needs some rework. Any suggestions are welcome. And double-checking the patch, I already have some changes to put in version 3. > > 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. Why do you need to synchronize it with 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. > I totally agree with you. I would have already dropped it if this patch didn't make sense. But since I seem to have understood that this is not the case, I do not understand why it cannot be improved in this series. The cover letter highlighted positive reactions to the series because the module had been requiring these kinds of changes for quite some time. So, why not take the opportunity to finalize this patch in this series even if it doesn't extend the supported features ? Anyway, I have some changes and tests to run before submitting version 3. If I don't get any hints before then, I'll drop it from the series. Thanks and regards, Dario > 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 */ > >
On Sat, 11 Jun 2022 12:46:04 +0200 Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > 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. > > Why do you need to synchronize it with drivers/net/slip/slip.c > implementation ? Because slcan.c is a derivative of slip.c and the code still looks *very* similar, so improvements in one file should be ported to the other and vice versa. This has happened several times now. > > When it has not real benefit and introduces more code and may create > > side effects, this beautification should probably be omitted at all. > > > > I totally agree with you. I would have already dropped it if this > patch didn't make sense. But since I seem to have understood that > this is not the case, I do not understand why it cannot be improved > in this series. This series is mostly about adding netlink support. If there is a point of contention about a beautification, it may be easier to discuss that separately, so the netlink code can be merged while the beautification is still being discussed. On another note, the global array of slcan_devs is really unnecessary and maintaining it is a mess - as seen in some of your patches, that have to account for it in tons of places and get complicated because of it. slcan_devs is probably grandfathered from a very old kernel, since slip.c is about 30 years old, so I suggest to remove it entirely. In fact, it may be easier to patch slcan_devs away first, and that will simplify your open/close patches - your decision :) If you wish to implement the slcan_devs removal, here are some hints: The private struct can just be allocated as part of struct can_priv in slcan_open(), like so: struct net_device *dev; dev = alloc_candev(sizeof(struct slcan), 0); And then accessed like so: struct slcan *sl = netdev_priv(dev); Make sure to add struct can_priv as the first member of struct slcan: /* This must be the first member when using alloc_candev() */ struct can_priv can; > The cover letter highlighted positive reactions to the series because > the module had been requiring these kinds of changes for quite > some time. So, why not take the opportunity to finalize this patch in > this series even if it doesn't extend the supported features ? Because... I can only speak for myself, but I'd merge all the unambiguous stuff first and discuss the difficult stuff later, if there are no interdependencies :) Max
On 12.06.22 18:23, Max Staudt wrote: > On Sat, 11 Jun 2022 12:46:04 +0200 > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > >>> 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. >> >> Why do you need to synchronize it with drivers/net/slip/slip.c >> implementation ? > > Because slcan.c is a derivative of slip.c and the code still looks > *very* similar, so improvements in one file should be ported to the > other and vice versa. This has happened several times now. > > >>> When it has not real benefit and introduces more code and may create >>> side effects, this beautification should probably be omitted at all. >>> >> >> I totally agree with you. I would have already dropped it if this >> patch didn't make sense. But since I seem to have understood that >> this is not the case, I do not understand why it cannot be improved >> in this series. > > This series is mostly about adding netlink support. If there is a point > of contention about a beautification, it may be easier to discuss that > separately, so the netlink code can be merged while the beautification > is still being discussed. > > > On another note, the global array of slcan_devs is really unnecessary > and maintaining it is a mess - as seen in some of your patches, that > have to account for it in tons of places and get complicated because of > it. > > slcan_devs is probably grandfathered from a very old kernel, since > slip.c is about 30 years old, so I suggest to remove it entirely. In > fact, it may be easier to patch slcan_devs away first, and that will > simplify your open/close patches - your decision :) > > > If you wish to implement the slcan_devs removal, here are some hints: > > The private struct can just be allocated as part of struct can_priv in > slcan_open(), like so: > > struct net_device *dev; > dev = alloc_candev(sizeof(struct slcan), 0); > > And then accessed like so: > > struct slcan *sl = netdev_priv(dev); > > Make sure to add struct can_priv as the first member of struct slcan: > > /* This must be the first member when using alloc_candev() */ > struct can_priv can; > > >> The cover letter highlighted positive reactions to the series because >> the module had been requiring these kinds of changes for quite >> some time. So, why not take the opportunity to finalize this patch in >> this series even if it doesn't extend the supported features ? > > Because... I can only speak for myself, but I'd merge all the > unambiguous stuff first and discuss the difficult stuff later, if there > are no interdependencies :) > > > > Max > Thanks for stepping in Max! Couldn't have summarized it better ;-) When I created slcan.c from slip.c this line discipline driver was just oriented at the SLIP idea including the user space tools to attach the network device to the serial tty. Therefore the driver took most of the mechanics (like the slcan_devs array) and did *only* the 'struct canframe' to ASCII conversion (and vice versa). @Dario: Implementing the CAN netlink API with open/close/bitrate-setting is a nice improvement. Especially as you wrote that you took care about the former/old API with slcan_attach/slcand. Best regards, Oliver
On Sun, Jun 12, 2022 at 7:13 PM Oliver Hartkopp <socketcan@hartkopp.net> wrote: > > > > On 12.06.22 18:23, Max Staudt wrote: > > On Sat, 11 Jun 2022 12:46:04 +0200 > > Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote: > > > >>> 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. > >> > >> Why do you need to synchronize it with drivers/net/slip/slip.c > >> implementation ? > > > > Because slcan.c is a derivative of slip.c and the code still looks > > *very* similar, so improvements in one file should be ported to the > > other and vice versa. This has happened several times now. > > > > > >>> When it has not real benefit and introduces more code and may create > >>> side effects, this beautification should probably be omitted at all. > >>> > >> > >> I totally agree with you. I would have already dropped it if this > >> patch didn't make sense. But since I seem to have understood that > >> this is not the case, I do not understand why it cannot be improved > >> in this series. > > > > This series is mostly about adding netlink support. If there is a point > > of contention about a beautification, it may be easier to discuss that > > separately, so the netlink code can be merged while the beautification > > is still being discussed. > > > > > > On another note, the global array of slcan_devs is really unnecessary > > and maintaining it is a mess - as seen in some of your patches, that > > have to account for it in tons of places and get complicated because of > > it. > > > > slcan_devs is probably grandfathered from a very old kernel, since > > slip.c is about 30 years old, so I suggest to remove it entirely. In > > fact, it may be easier to patch slcan_devs away first, and that will > > simplify your open/close patches - your decision :) > > > > > > If you wish to implement the slcan_devs removal, here are some hints: > > > > The private struct can just be allocated as part of struct can_priv in > > slcan_open(), like so: > > > > struct net_device *dev; > > dev = alloc_candev(sizeof(struct slcan), 0); > > > > And then accessed like so: > > > > struct slcan *sl = netdev_priv(dev); > > > > Make sure to add struct can_priv as the first member of struct slcan: > > > > /* This must be the first member when using alloc_candev() */ > > struct can_priv can; > > > > > >> The cover letter highlighted positive reactions to the series because > >> the module had been requiring these kinds of changes for quite > >> some time. So, why not take the opportunity to finalize this patch in > >> this series even if it doesn't extend the supported features ? > > > > Because... I can only speak for myself, but I'd merge all the > > unambiguous stuff first and discuss the difficult stuff later, if there > > are no interdependencies :) > > > > > > > > Max > > > > Thanks for stepping in Max! > > Couldn't have summarized it better ;-) > > When I created slcan.c from slip.c this line discipline driver was just > oriented at the SLIP idea including the user space tools to attach the > network device to the serial tty. > > Therefore the driver took most of the mechanics (like the slcan_devs > array) and did *only* the 'struct canframe' to ASCII conversion (and > vice versa). > > @Dario: Implementing the CAN netlink API with open/close/bitrate-setting > is a nice improvement. Especially as you wrote that you took care about > the former/old API with slcan_attach/slcand. > > Best regards, > Oliver Thanks to both of you for the explanations. best regards, Dario
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 */
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(-)