| Message ID | 1414579527-31100-5-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/29/2014 11:45 AM, Dong Aisheng wrote: > Currently priv->ctrlmode is not cleared when close_candev, so next time > the driver will still use this value to set controller even user > does not set any ctrl mode. > e.g. > Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on > Controller will be in loopback mode > Step 2. ip link set can0 down > Step 3. ip link set can0 up type can0 bitrate 1000000 > Controller will still be set to loopback mode in driver due to saved > priv->ctrlmode. > > This patch clears priv->ctrlmode when the CAN interface is closed, > and set it to correct mode according to next user setting. Oliver, what do you think of this patch? It will introduce a subtle change to the userspace. Marc > Signed-off-by: Dong Aisheng <b29396@freescale.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 02492d2..1fce485 100644 > --- a/drivers/net/can/dev.c > +++ b/drivers/net/can/dev.c > @@ -671,6 +671,7 @@ void close_candev(struct net_device *dev) > > del_timer_sync(&priv->restart_timer); > can_flush_echo_skb(dev); > + priv->ctrlmode = 0; > } > EXPORT_SYMBOL_GPL(close_candev); > >
On 11/03/2014 05:28 PM, Marc Kleine-Budde wrote: > On 10/29/2014 11:45 AM, Dong Aisheng wrote: >> Currently priv->ctrlmode is not cleared when close_candev, so next time >> the driver will still use this value to set controller even user >> does not set any ctrl mode. >> e.g. >> Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on >> Controller will be in loopback mode >> Step 2. ip link set can0 down >> Step 3. ip link set can0 up type can0 bitrate 1000000 >> Controller will still be set to loopback mode in driver due to saved >> priv->ctrlmode. >> >> This patch clears priv->ctrlmode when the CAN interface is closed, >> and set it to correct mode according to next user setting. > > Oliver, what do you think of this patch? It will introduce a subtle > change to the userspace. Hi Marc, indeed the current policy is that ifdown/ifup doesn't change any setting. When we start to clear priv->ctrlmode when setting the interface down the question arises whether the bitrate(s) has/have to be wiped too. Setting the interface down/up can be done with ip or ifconfig without changing the controller settings. So IMO we can not assume that people always use the 'all-in-one' ip link set can0 up type can0 bitrate 1000000 command as given in the example above. You might also do it like this: ip link set can0 up type can0 bitrate 1000000 loopback on ip link set can0 down ip link set can0 type can0 loopback off ip link set can0 up Finally I would vote to reject this patch to stay consistent. When users like to fiddle with options like loopback they need to take care about their 'expert' settings too. Regards, Oliver > >> Signed-off-by: Dong Aisheng <b29396@freescale.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 02492d2..1fce485 100644 >> --- a/drivers/net/can/dev.c >> +++ b/drivers/net/can/dev.c >> @@ -671,6 +671,7 @@ void close_candev(struct net_device *dev) >> >> del_timer_sync(&priv->restart_timer); >> can_flush_echo_skb(dev); >> + priv->ctrlmode = 0; >> } >> EXPORT_SYMBOL_GPL(close_candev); >> >> > >
On 10/29/2014 11:45 AM, Dong Aisheng wrote: > Currently priv->ctrlmode is not cleared when close_candev, so next time > the driver will still use this value to set controller even user > does not set any ctrl mode. > e.g. > Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on > Controller will be in loopback mode > Step 2. ip link set can0 down > Step 3. ip link set can0 up type can0 bitrate 1000000 > Controller will still be set to loopback mode in driver due to saved > priv->ctrlmode. > > This patch clears priv->ctrlmode when the CAN interface is closed, > and set it to correct mode according to next user setting. > > Signed-off-by: Dong Aisheng <b29396@freescale.com> NACK, as discussed with Oliver. Marc
On Mon, Nov 03, 2014 at 09:47:22PM +0100, Marc Kleine-Budde wrote: > On 10/29/2014 11:45 AM, Dong Aisheng wrote: > > Currently priv->ctrlmode is not cleared when close_candev, so next time > > the driver will still use this value to set controller even user > > does not set any ctrl mode. > > e.g. > > Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on > > Controller will be in loopback mode > > Step 2. ip link set can0 down > > Step 3. ip link set can0 up type can0 bitrate 1000000 > > Controller will still be set to loopback mode in driver due to saved > > priv->ctrlmode. > > > > This patch clears priv->ctrlmode when the CAN interface is closed, > > and set it to correct mode according to next user setting. > > > > Signed-off-by: Dong Aisheng <b29396@freescale.com> > > NACK, as discussed with Oliver. > Okay, i'm fine with it. Regards Dong Aisheng > 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 | >
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index 02492d2..1fce485 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -671,6 +671,7 @@ void close_candev(struct net_device *dev) del_timer_sync(&priv->restart_timer); can_flush_echo_skb(dev); + priv->ctrlmode = 0; } EXPORT_SYMBOL_GPL(close_candev);
Currently priv->ctrlmode is not cleared when close_candev, so next time the driver will still use this value to set controller even user does not set any ctrl mode. e.g. Step 1. ip link set can0 up type can0 bitrate 1000000 loopback on Controller will be in loopback mode Step 2. ip link set can0 down Step 3. ip link set can0 up type can0 bitrate 1000000 Controller will still be set to loopback mode in driver due to saved priv->ctrlmode. This patch clears priv->ctrlmode when the CAN interface is closed, and set it to correct mode according to next user setting. Signed-off-by: Dong Aisheng <b29396@freescale.com> --- drivers/net/can/dev.c | 1 + 1 file changed, 1 insertion(+)