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

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(-)
  

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