[PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Joakim Zhang]

CAN driver always writes registers according to "ctrlmode", and now CAN
framework will keep CAN ctrlmode after device closed.

e.g. with following sequences:
1)ip link set can0 type can bitrate 1000000 loopback on
2)ip link set can0 up
3)ip link set can0 down
4)ip link set can0 type can bitrate 1000000
5)ip link set can0 up

After this sequence, we may want to test loopback for the first time and
not to test loopback second time. However, CAN device still keep
"ctrlmode" as loopback on.

This patch intends to clean up CAN ctrlmode when close CAN device. We
can set which ctrlmode we need when open CAN device again.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/can/dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c05e4d50d43d..4f69a8f16ba3 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -870,6 +870,7 @@ void close_candev(struct net_device *dev)
 	struct can_priv *priv = netdev_priv(dev);
 
 	cancel_delayed_work_sync(&priv->restart_work);
+	priv->ctrlmode = 0;
 	can_flush_echo_skb(dev);
 }
 EXPORT_SYMBOL_GPL(close_candev);
-- 
2.17.1

Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1779 bytes --]

On 4/10/19 8:27 AM, Joakim Zhang wrote:
> CAN driver always writes registers according to "ctrlmode", and now CAN
> framework will keep CAN ctrlmode after device closed.
> 
> e.g. with following sequences:
> 1)ip link set can0 type can bitrate 1000000 loopback on
> 2)ip link set can0 up
> 3)ip link set can0 down
> 4)ip link set can0 type can bitrate 1000000
> 5)ip link set can0 up
> 
> After this sequence, we may want to test loopback for the first time and
> not to test loopback second time. However, CAN device still keep
> "ctrlmode" as loopback on.

Then you should configure loopback off in userspace.

> This patch intends to clean up CAN ctrlmode when close CAN device. We
> can set which ctrlmode we need when open CAN device again.

Consider a CANFD device, where CANFD has been enabled. It will feel very
weird if the interface looses the CANFD property by just a ifdown; ifup.

> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/can/dev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index c05e4d50d43d..4f69a8f16ba3 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -870,6 +870,7 @@ void close_candev(struct net_device *dev)
>  	struct can_priv *priv = netdev_priv(dev);
>  
>  	cancel_delayed_work_sync(&priv->restart_work);
> +	priv->ctrlmode = 0;
>  	can_flush_echo_skb(dev);
>  }
>  EXPORT_SYMBOL_GPL(close_candev);
> 

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年4月10日 15:19
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; wg@grandegger.com;
> davem@davemloft.net
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN
> device
> 
> On 4/10/19 8:27 AM, Joakim Zhang wrote:
> > CAN driver always writes registers according to "ctrlmode", and now
> > CAN framework will keep CAN ctrlmode after device closed.
> >
> > e.g. with following sequences:
> > 1)ip link set can0 type can bitrate 1000000 loopback on 2)ip link set
> > can0 up 3)ip link set can0 down 4)ip link set can0 type can bitrate
> > 1000000 5)ip link set can0 up
> >
> > After this sequence, we may want to test loopback for the first time
> > and not to test loopback second time. However, CAN device still keep
> > "ctrlmode" as loopback on.
> 
> Then you should configure loopback off in userspace.

 Got it. Thank you.

> > This patch intends to clean up CAN ctrlmode when close CAN device. We
> > can set which ctrlmode we need when open CAN device again.
> 
> Consider a CANFD device, where CANFD has been enabled. It will feel very
> weird if the interface looses the CANFD property by just a ifdown; ifup.

Yes, you are right. But fd mode should compatible with normal can. When I want to switch to normal can from fd mode.
You mean that we should configure fd off in userspace as we may write some specific registers only by fd mode?


Best Regards,
Joakim Zhang
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/can/dev.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index
> > c05e4d50d43d..4f69a8f16ba3 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -870,6 +870,7 @@ void close_candev(struct net_device *dev)
> >  	struct can_priv *priv = netdev_priv(dev);
> >
> >  	cancel_delayed_work_sync(&priv->restart_work);
> > +	priv->ctrlmode = 0;
> >  	can_flush_echo_skb(dev);
> >  }
> >  EXPORT_SYMBOL_GPL(close_candev);
> >
> 
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Re: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2238 bytes --]

On 4/10/19 9:55 AM, Joakim Zhang wrote:
> 
>> -----Original Message-----
>> From: Marc Kleine-Budde <mkl@pengutronix.de>
>> Sent: 2019年4月10日 15:19
>> To: Joakim Zhang <qiangqing.zhang@nxp.com>; wg@grandegger.com;
>> davem@davemloft.net
>> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-can@vger.kernel.org;
>> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN
>> device
>>
>> On 4/10/19 8:27 AM, Joakim Zhang wrote:
>>> CAN driver always writes registers according to "ctrlmode", and now
>>> CAN framework will keep CAN ctrlmode after device closed.
>>>
>>> e.g. with following sequences:
>>> 1)ip link set can0 type can bitrate 1000000 loopback on 2)ip link set
>>> can0 up 3)ip link set can0 down 4)ip link set can0 type can bitrate
>>> 1000000 5)ip link set can0 up
>>>
>>> After this sequence, we may want to test loopback for the first time
>>> and not to test loopback second time. However, CAN device still keep
>>> "ctrlmode" as loopback on.
>>
>> Then you should configure loopback off in userspace.
> 
>  Got it. Thank you.
> 
>>> This patch intends to clean up CAN ctrlmode when close CAN device. We
>>> can set which ctrlmode we need when open CAN device again.
>>
>> Consider a CANFD device, where CANFD has been enabled. It will feel very
>> weird if the interface looses the CANFD property by just a ifdown; ifup.
> 
> Yes, you are right. But fd mode should compatible with normal can.

CANFD was just an example. Take CAN_CTRLMODE_ONE_SHOT as another
example. You don't want your network device settings to vanish if you
switch the interface off and on again.

> When I want to switch to normal can from fd mode. You mean that we
> should configure fd off in userspace as we may write some specific
> registers only by fd mode?

Whatever mode you enable via user space use user space to disable them
again.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Oliver Hartkopp]

On 10.04.19 09:19, Marc Kleine-Budde wrote:
> On 4/10/19 8:27 AM, Joakim Zhang wrote:
>> CAN driver always writes registers according to "ctrlmode", and now CAN
>> framework will keep CAN ctrlmode after device closed.
>>
>> e.g. with following sequences:
>> 1)ip link set can0 type can bitrate 1000000 loopback on
>> 2)ip link set can0 up
>> 3)ip link set can0 down
>> 4)ip link set can0 type can bitrate 1000000
>> 5)ip link set can0 up
>>
>> After this sequence, we may want to test loopback for the first time and
>> not to test loopback second time. However, CAN device still keep
>> "ctrlmode" as loopback on.
> 
> Then you should configure loopback off in userspace.

ACK!

The idea is that you can set a CAN interface like this:

ip link set can0 type can bitrate 1000000 loopback on

(..)

ip link set can0 down
ip link set can0 up

and none of the configuration changes in the meanwhile.

Regards,
Oliver

> 
>> This patch intends to clean up CAN ctrlmode when close CAN device. We
>> can set which ctrlmode we need when open CAN device again.
> 
> Consider a CANFD device, where CANFD has been enabled. It will feel very
> weird if the interface looses the CANFD property by just a ifdown; ifup.
> 
>>
>> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
>> ---
>>   drivers/net/can/dev.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
>> index c05e4d50d43d..4f69a8f16ba3 100644
>> --- a/drivers/net/can/dev.c
>> +++ b/drivers/net/can/dev.c
>> @@ -870,6 +870,7 @@ void close_candev(struct net_device *dev)
>>   	struct can_priv *priv = netdev_priv(dev);
>>   
>>   	cancel_delayed_work_sync(&priv->restart_work);
>> +	priv->ctrlmode = 0;
>>   	can_flush_echo_skb(dev);
>>   }
>>   EXPORT_SYMBOL_GPL(close_candev);
>>
> 
> Marc
> 

RE: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年4月11日 1:50
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; wg@grandegger.com;
> davem@davemloft.net
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close
> CAN device
> 
> On 4/10/19 9:55 AM, Joakim Zhang wrote:
> >
> >> -----Original Message-----
> >> From: Marc Kleine-Budde <mkl@pengutronix.de>
> >> Sent: 2019年4月10日 15:19
> >> To: Joakim Zhang <qiangqing.zhang@nxp.com>; wg@grandegger.com;
> >> davem@davemloft.net
> >> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-can@vger.kernel.org;
> >> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close
> >> CAN device
> >>
> >> On 4/10/19 8:27 AM, Joakim Zhang wrote:
> >>> CAN driver always writes registers according to "ctrlmode", and now
> >>> CAN framework will keep CAN ctrlmode after device closed.
> >>>
> >>> e.g. with following sequences:
> >>> 1)ip link set can0 type can bitrate 1000000 loopback on 2)ip link
> >>> set
> >>> can0 up 3)ip link set can0 down 4)ip link set can0 type can bitrate
> >>> 1000000 5)ip link set can0 up
> >>>
> >>> After this sequence, we may want to test loopback for the first time
> >>> and not to test loopback second time. However, CAN device still keep
> >>> "ctrlmode" as loopback on.
> >>
> >> Then you should configure loopback off in userspace.
> >
> >  Got it. Thank you.
> >
> >>> This patch intends to clean up CAN ctrlmode when close CAN device.
> >>> We can set which ctrlmode we need when open CAN device again.
> >>
> >> Consider a CANFD device, where CANFD has been enabled. It will feel
> >> very weird if the interface looses the CANFD property by just a ifdown; ifup.
> >
> > Yes, you are right. But fd mode should compatible with normal can.
> 
> CANFD was just an example. Take CAN_CTRLMODE_ONE_SHOT as another
> example. You don't want your network device settings to vanish if you switch
> the interface off and on again.
> 
> > When I want to switch to normal can from fd mode. You mean that we
> > should configure fd off in userspace as we may write some specific
> > registers only by fd mode?
> 
> Whatever mode you enable via user space use user space to disable them
> again.

Thank you for your detailed explanation. But in flexcan driver, it just set register bit when mode on and do not clear
register bit when mode off, e.g. loopback, listennoly, 3 samples....
When we configure mode off after configured mode on in the user space, corresponding bit will not change. Is this should be improved in flexcan driver?
 
Best Regards,
Joakim Zhang
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

Re: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1972 bytes --]

On 4/11/19 3:16 AM, Joakim Zhang wrote:
>> Whatever mode you enable via user space use user space to disable them
>> again.
> 
> Thank you for your detailed explanation. But in flexcan driver, it
> just set register bit when mode on and do not clear register bit when
> mode off, e.g. loopback, listennoly, 3 samples....

Let's see:

> 	reg = priv->read(&regs->ctrl);

The ctrl reg is read

> 	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
> 		 FLEXCAN_CTRL_RJW(0x3) |
> 		 FLEXCAN_CTRL_PSEG1(0x7) |
> 		 FLEXCAN_CTRL_PSEG2(0x7) |
> 		 FLEXCAN_CTRL_PROPSEG(0x7) |
> 		 FLEXCAN_CTRL_LPB |
> 		 FLEXCAN_CTRL_SMP |
> 		 FLEXCAN_CTRL_LOM);

....the loopback mode, 3 sample mode and listen only mode bits are
masked out....

> 	reg |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) |
> 		FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) |
> 		FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) |
> 		FLEXCAN_CTRL_RJW(bt->sjw - 1) |
> 		FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1);
> 
> 	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> 		reg |= FLEXCAN_CTRL_LPB;
> 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> 		reg |= FLEXCAN_CTRL_LOM;
> 	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> 		reg |= FLEXCAN_CTRL_SMP;

...and the bits are set, if enabled by user space...

> 	netdev_dbg(dev, "writing ctrl=0x%08x\n", reg);
> 	priv->write(reg, &regs->ctrl);

...and written to the register.

So all three bits will be unset, if unset by user space, right?

> When we configure mode off after configured mode on in the user
> space, corresponding bit will not change. Is this should be improved
> in flexcan driver?

That should work. If it doesn't work, we need a fix.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close CAN device

[Joakim Zhang]

> -----Original Message-----
> From: Marc Kleine-Budde <mkl@pengutronix.de>
> Sent: 2019年4月11日 14:39
> To: Joakim Zhang <qiangqing.zhang@nxp.com>; wg@grandegger.com;
> davem@davemloft.net
> Cc: dl-linux-imx <linux-imx@nxp.com>; linux-can@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH] can: dev: clean up CAN ctrlmode when close
> CAN device
> 
> On 4/11/19 3:16 AM, Joakim Zhang wrote:
> >> Whatever mode you enable via user space use user space to disable
> >> them again.
> >
> > Thank you for your detailed explanation. But in flexcan driver, it
> > just set register bit when mode on and do not clear register bit when
> > mode off, e.g. loopback, listennoly, 3 samples....
> 
> Let's see:
> 
> > 	reg = priv->read(&regs->ctrl);
> 
> The ctrl reg is read
> 
> > 	reg &= ~(FLEXCAN_CTRL_PRESDIV(0xff) |
> > 		 FLEXCAN_CTRL_RJW(0x3) |
> > 		 FLEXCAN_CTRL_PSEG1(0x7) |
> > 		 FLEXCAN_CTRL_PSEG2(0x7) |
> > 		 FLEXCAN_CTRL_PROPSEG(0x7) |
> > 		 FLEXCAN_CTRL_LPB |
> > 		 FLEXCAN_CTRL_SMP |
> > 		 FLEXCAN_CTRL_LOM);
> 
> ....the loopback mode, 3 sample mode and listen only mode bits are masked
> out....
> 
> > 	reg |= FLEXCAN_CTRL_PRESDIV(bt->brp - 1) |
> > 		FLEXCAN_CTRL_PSEG1(bt->phase_seg1 - 1) |
> > 		FLEXCAN_CTRL_PSEG2(bt->phase_seg2 - 1) |
> > 		FLEXCAN_CTRL_RJW(bt->sjw - 1) |
> > 		FLEXCAN_CTRL_PROPSEG(bt->prop_seg - 1);
> >
> > 	if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > 		reg |= FLEXCAN_CTRL_LPB;
> > 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > 		reg |= FLEXCAN_CTRL_LOM;
> > 	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> > 		reg |= FLEXCAN_CTRL_SMP;
> 
> ...and the bits are set, if enabled by user space...
> 
> > 	netdev_dbg(dev, "writing ctrl=0x%08x\n", reg);
> > 	priv->write(reg, &regs->ctrl);
> 
> ...and written to the register.
> 
> So all three bits will be unset, if unset by user space, right?

Oh, I see. Thank you!

Best Regards,
Joakim Zhang
> 
> > When we configure mode off after configured mode on in the user space,
> > corresponding bit will not change. Is this should be improved in
> > flexcan driver?
> 
> That should work. If it doesn't work, we need a fix.
> 
> regards,
> Marc
> 
> --
> Pengutronix e.K.                  | Marc Kleine-Budde           |
> Industrial Linux Solutions        | Phone: +49-231-2826-924     |
> Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |