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

[Dario Binacchi <dario.binacchi@amarulasolutions.com>]

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

-- 

Dario Binacchi

Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com