[PATCH v4 6/6] can: handle bus-off in can_change_state()

[PATCH v4 6/6] can: handle bus-off in can_change_state()

[Andri Yngvason]

Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
---
 drivers/net/can/dev.c             |  1 +
 drivers/net/can/flexcan.c         |  3 ---
 drivers/net/can/mscan/mscan.c     | 11 +++++------
 drivers/net/can/sja1000/sja1000.c |  3 ---
 4 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3e2da66..17fce72 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -340,6 +340,7 @@ void can_change_state(struct net_device *dev, struct can_frame *cf,
 
 	if (unlikely(new_state == CAN_STATE_BUS_OFF)) {
 		cf->can_id |= CAN_ERR_BUSOFF;
+		can_bus_off(dev);
 		return;
 	}
 
diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index 9f91735..0823956 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -612,9 +612,6 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
 
 	can_change_state(dev, cf, tx_state, rx_state);
 
-	if (unlikely(new_state == CAN_STATE_BUS_OFF))
-		can_bus_off(dev);
-
 	netif_receive_skb(skb);
 
 	dev->stats.rx_packets++;
diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
index e36b740..2eb84f1 100644
--- a/drivers/net/can/mscan/mscan.c
+++ b/drivers/net/can/mscan/mscan.c
@@ -362,11 +362,7 @@ static void mscan_get_err_frame(struct net_device *dev, struct can_frame *frame,
 
 	new_state = get_new_state(dev, canrflg);
 	if (new_state != priv->can.state) {
-		can_change_state(dev, frame,
-				 state_map[MSCAN_STATE_TX(canrflg)],
-				 state_map[MSCAN_STATE_RX(canrflg)]);
-
-		if (priv->can.state == CAN_STATE_BUS_OFF) {
+		if (new_state == CAN_STATE_BUS_OFF) {
 			/*
 			 * The MSCAN on the MPC5200 does recover from bus-off
 			 * automatically. To avoid that we stop the chip doing
@@ -378,8 +374,11 @@ static void mscan_get_err_frame(struct net_device *dev, struct can_frame *frame,
 				setbits8(&regs->canctl0,
 					 MSCAN_SLPRQ | MSCAN_INITRQ);
 			}
-			can_bus_off(dev);
 		}
+
+		can_change_state(dev, frame,
+				 state_map[MSCAN_STATE_TX(canrflg)],
+				 state_map[MSCAN_STATE_RX(canrflg)]);
 	}
 	priv->shadow_statflg = canrflg & MSCAN_STAT_MSK;
 	frame->can_dlc = CAN_ERR_DLC;
diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c
index 32bd7f4..37b6392 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -479,9 +479,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 		rx_state = txerr <= rxerr ? state : 0;
 
 		can_change_state(dev, cf, tx_state, rx_state);
-
-		if(state == CAN_STATE_BUS_OFF)
-			can_bus_off(dev);
 	}
 
 	netif_rx(skb);
-- 
1.9.1

Re: [PATCH v4 6/6] can: handle bus-off in can_change_state()

[Wolfgang Grandegger]

On Fri, 28 Nov 2014 12:12:28 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> ---
>  drivers/net/can/dev.c             |  1 +
>  drivers/net/can/flexcan.c         |  3 ---
>  drivers/net/can/mscan/mscan.c     | 11 +++++------
>  drivers/net/can/sja1000/sja1000.c |  3 ---
>  4 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 3e2da66..17fce72 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -340,6 +340,7 @@ void can_change_state(struct net_device *dev, struct
> can_frame *cf,
>  
>  	if (unlikely(new_state == CAN_STATE_BUS_OFF)) {
>  		cf->can_id |= CAN_ERR_BUSOFF;
> +		can_bus_off(dev);
>  		return;
>  	}
>  
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 9f91735..0823956 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -612,9 +612,6 @@ static int flexcan_poll_state(struct net_device
*dev,
> u32 reg_esr)
>  
>  	can_change_state(dev, cf, tx_state, rx_state);
>  
> -	if (unlikely(new_state == CAN_STATE_BUS_OFF))
> -		can_bus_off(dev);
> -
>  	netif_receive_skb(skb);
>  
>  	dev->stats.rx_packets++;
> diff --git a/drivers/net/can/mscan/mscan.c
b/drivers/net/can/mscan/mscan.c
> index e36b740..2eb84f1 100644
> --- a/drivers/net/can/mscan/mscan.c
> +++ b/drivers/net/can/mscan/mscan.c
> @@ -362,11 +362,7 @@ static void mscan_get_err_frame(struct net_device
> *dev, struct can_frame *frame,
>  
>  	new_state = get_new_state(dev, canrflg);
>  	if (new_state != priv->can.state) {
> -		can_change_state(dev, frame,
> -				 state_map[MSCAN_STATE_TX(canrflg)],
> -				 state_map[MSCAN_STATE_RX(canrflg)]);
> -
> -		if (priv->can.state == CAN_STATE_BUS_OFF) {
> +		if (new_state == CAN_STATE_BUS_OFF) {
>  			/*
>  			 * The MSCAN on the MPC5200 does recover from bus-off
>  			 * automatically. To avoid that we stop the chip doing
> @@ -378,8 +374,11 @@ static void mscan_get_err_frame(struct net_device
> *dev, struct can_frame *frame,
>  				setbits8(&regs->canctl0,
>  					 MSCAN_SLPRQ | MSCAN_INITRQ);
>  			}
> -			can_bus_off(dev);
>  		}
> +
> +		can_change_state(dev, frame,
> +				 state_map[MSCAN_STATE_TX(canrflg)],
> +				 state_map[MSCAN_STATE_RX(canrflg)]);
>  	}
>  	priv->shadow_statflg = canrflg & MSCAN_STAT_MSK;
>  	frame->can_dlc = CAN_ERR_DLC;
> diff --git a/drivers/net/can/sja1000/sja1000.c
> b/drivers/net/can/sja1000/sja1000.c
> index 32bd7f4..37b6392 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -479,9 +479,6 @@ static int sja1000_err(struct net_device *dev,
uint8_t
> isrc, uint8_t status)
>  		rx_state = txerr <= rxerr ? state : 0;
>  
>  		can_change_state(dev, cf, tx_state, rx_state);
> -
> -		if(state == CAN_STATE_BUS_OFF)
> -			can_bus_off(dev);

Ah, no, that's wrong because not all devices use this function. I thought
just moving the counter incrementation out of can_bus_off() back to
the driver (for all drivers not yet using can_change_state). Sorry for
confusion.

Wolfgang.

Re: [PATCH v4 6/6] can: handle bus-off in can_change_state()

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-11-28 12:21:53)
> On Fri, 28 Nov 2014 12:12:28 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:
> > Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> > ---
> >  drivers/net/can/dev.c             |  1 +
> >  drivers/net/can/flexcan.c         |  3 ---
> >  drivers/net/can/mscan/mscan.c     | 11 +++++------
> >  drivers/net/can/sja1000/sja1000.c |  3 ---
> >  4 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> > index 3e2da66..17fce72 100644
> > --- a/drivers/net/can/dev.c
> > +++ b/drivers/net/can/dev.c
> > @@ -340,6 +340,7 @@ void can_change_state(struct net_device *dev, struct
> > can_frame *cf,
> >  
> >       if (unlikely(new_state == CAN_STATE_BUS_OFF)) {
> >               cf->can_id |= CAN_ERR_BUSOFF;
> > +             can_bus_off(dev);
> >               return;
> >       }
> >  
> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> > index 9f91735..0823956 100644
> > --- a/drivers/net/can/flexcan.c
> > +++ b/drivers/net/can/flexcan.c
> > @@ -612,9 +612,6 @@ static int flexcan_poll_state(struct net_device
> *dev,
> > u32 reg_esr)
> >  
> >       can_change_state(dev, cf, tx_state, rx_state);
> >  
> > -     if (unlikely(new_state == CAN_STATE_BUS_OFF))
> > -             can_bus_off(dev);
> > -
> >       netif_receive_skb(skb);
> >  
> >       dev->stats.rx_packets++;
> > diff --git a/drivers/net/can/mscan/mscan.c
> b/drivers/net/can/mscan/mscan.c
> > index e36b740..2eb84f1 100644
> > --- a/drivers/net/can/mscan/mscan.c
> > +++ b/drivers/net/can/mscan/mscan.c
> > @@ -362,11 +362,7 @@ static void mscan_get_err_frame(struct net_device
> > *dev, struct can_frame *frame,
> >  
> >       new_state = get_new_state(dev, canrflg);
> >       if (new_state != priv->can.state) {
> > -             can_change_state(dev, frame,
> > -                              state_map[MSCAN_STATE_TX(canrflg)],
> > -                              state_map[MSCAN_STATE_RX(canrflg)]);
> > -
> > -             if (priv->can.state == CAN_STATE_BUS_OFF) {
> > +             if (new_state == CAN_STATE_BUS_OFF) {
> >                       /*
> >                        * The MSCAN on the MPC5200 does recover from bus-off
> >                        * automatically. To avoid that we stop the chip doing
> > @@ -378,8 +374,11 @@ static void mscan_get_err_frame(struct net_device
> > *dev, struct can_frame *frame,
> >                               setbits8(&regs->canctl0,
> >                                        MSCAN_SLPRQ | MSCAN_INITRQ);
> >                       }
> > -                     can_bus_off(dev);
> >               }
> > +
> > +             can_change_state(dev, frame,
> > +                              state_map[MSCAN_STATE_TX(canrflg)],
> > +                              state_map[MSCAN_STATE_RX(canrflg)]);
> >       }
> >       priv->shadow_statflg = canrflg & MSCAN_STAT_MSK;
> >       frame->can_dlc = CAN_ERR_DLC;
> > diff --git a/drivers/net/can/sja1000/sja1000.c
> > b/drivers/net/can/sja1000/sja1000.c
> > index 32bd7f4..37b6392 100644
> > --- a/drivers/net/can/sja1000/sja1000.c
> > +++ b/drivers/net/can/sja1000/sja1000.c
> > @@ -479,9 +479,6 @@ static int sja1000_err(struct net_device *dev,
> uint8_t
> > isrc, uint8_t status)
> >               rx_state = txerr <= rxerr ? state : 0;
> >  
> >               can_change_state(dev, cf, tx_state, rx_state);
> > -
> > -             if(state == CAN_STATE_BUS_OFF)
> > -                     can_bus_off(dev);
> 
> Ah, no, that's wrong because not all devices use this function. I thought
> just moving the counter incrementation out of can_bus_off() back to
> the driver (for all drivers not yet using can_change_state). Sorry for
> confusion.

There wasn't any confusion. I just felt like this might be a good idea, so I
included it. Turns out it isn't. It's a separate patch, so it can be easily
omitted.

What you described is in patch 5.

--
Andri.