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

[Oliver Hartkopp]

On 12.06.22 18:23, Max Staudt wrote:
> On Sat, 11 Jun 2022 12:46:04 +0200
> Dario Binacchi <dario.binacchi@xxxxxxxxxxxxxxxxxxxx> 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