Re: [PATCH v3 4/4] can: flexcan: Consolidate and unify state change handling

[Wolfgang Grandegger <wg@grandegger.com>]

On 09/26/2014 07:31 PM, Andri Yngvason wrote:
> Replacing error state change handling with the new mechanism.
> 
> Changes made since last proposal:
> can: flexcan: improve rx/tx state recognition.

Move below "---", please.

> Note: Flexcan is problematic because there is no interrupt generated when
> transitioning between error-active and error-passive. If the bus is
> disconnected, there is no event that will trigger a napi poll, so the new state
> will not be discovered until the bus is connected. This problem goes away if
> berr-reporting is enabled, because then the ack-error takes care of it.

Well, this is a known issue and should be addressed via platform data
(using the FLEXCAN_HAS_BROKEN_ERR_STATE flags).

> 
> Another problem is that if nothing is happening on the bus, when it's connected
> again, we won't see the new state until someone transmits something on the bus.
> 
> I'd say that these are minor issues because a "healthy" bus has traffic and the
> "disconnected bus" issue has a work-around.

Would be nice to do some tests on a more recent Flexcan core as well.

> 
> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> ---
>  drivers/net/can/flexcan.c | 101 +++++++++-------------------------------------
>  1 file changed, 18 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 60f86bd..e7dea64 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -577,98 +577,30 @@ static int flexcan_poll_bus_err(struct net_device *dev, u32 reg_esr)
>  	return 1;
>  }
>  
> -static void do_state(struct net_device *dev,
> -		     struct can_frame *cf, enum can_state new_state)
> -{
> -	struct flexcan_priv *priv = netdev_priv(dev);
> -	struct can_berr_counter bec;
> -
> -	__flexcan_get_berr_counter(dev, &bec);
> -
> -	switch (priv->can.state) {
> -	case CAN_STATE_ERROR_ACTIVE:
> -		/*
> -		 * from: ERROR_ACTIVE
> -		 * to  : ERROR_WARNING, ERROR_PASSIVE, BUS_OFF
> -		 * =>  : there was a warning int
> -		 */
> -		if (new_state >= CAN_STATE_ERROR_WARNING &&
> -		    new_state <= CAN_STATE_BUS_OFF) {
> -			netdev_dbg(dev, "Error Warning IRQ\n");
> -			priv->can.can_stats.error_warning++;
> -
> -			cf->can_id |= CAN_ERR_CRTL;
> -			cf->data[1] = (bec.txerr > bec.rxerr) ?
> -				CAN_ERR_CRTL_TX_WARNING :
> -				CAN_ERR_CRTL_RX_WARNING;
> -		}
> -	case CAN_STATE_ERROR_WARNING:	/* fallthrough */
> -		/*
> -		 * from: ERROR_ACTIVE, ERROR_WARNING
> -		 * to  : ERROR_PASSIVE, BUS_OFF
> -		 * =>  : error passive int
> -		 */
> -		if (new_state >= CAN_STATE_ERROR_PASSIVE &&
> -		    new_state <= CAN_STATE_BUS_OFF) {
> -			netdev_dbg(dev, "Error Passive IRQ\n");
> -			priv->can.can_stats.error_passive++;
> -
> -			cf->can_id |= CAN_ERR_CRTL;
> -			cf->data[1] = (bec.txerr > bec.rxerr) ?
> -				CAN_ERR_CRTL_TX_PASSIVE :
> -				CAN_ERR_CRTL_RX_PASSIVE;
> -		}
> -		break;
> -	case CAN_STATE_BUS_OFF:
> -		netdev_err(dev, "BUG! "
> -			   "hardware recovered automatically from BUS_OFF\n");
> -		break;
> -	default:
> -		break;
> -	}
> -
> -	/* process state changes depending on the new state */
> -	switch (new_state) {
> -	case CAN_STATE_ERROR_WARNING:
> -		netdev_dbg(dev, "Error Warning\n");
> -		cf->can_id |= CAN_ERR_CRTL;
> -		cf->data[1] = (bec.txerr > bec.rxerr) ?
> -			CAN_ERR_CRTL_TX_WARNING :
> -			CAN_ERR_CRTL_RX_WARNING;
> -		break;
> -	case CAN_STATE_ERROR_ACTIVE:
> -		netdev_dbg(dev, "Error Active\n");
> -		cf->can_id |= CAN_ERR_PROT;
> -		cf->data[2] = CAN_ERR_PROT_ACTIVE;
> -		break;
> -	case CAN_STATE_BUS_OFF:
> -		cf->can_id |= CAN_ERR_BUSOFF;
> -		can_bus_off(dev);
> -		break;
> -	default:
> -		break;
> -	}
> -}
> -
>  static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
>  {
>  	struct flexcan_priv *priv = netdev_priv(dev);
>  	struct sk_buff *skb;
>  	struct can_frame *cf;
> -	enum can_state new_state;
> +	enum can_state new_state = 0, rx_state = 0, tx_state = 0;
>  	int flt;
> +	struct can_berr_counter bec;
>  
>  	flt = reg_esr & FLEXCAN_ESR_FLT_CONF_MASK;
>  	if (likely(flt == FLEXCAN_ESR_FLT_CONF_ACTIVE)) {
> -		if (likely(!(reg_esr & (FLEXCAN_ESR_TX_WRN |
> -					FLEXCAN_ESR_RX_WRN))))
> -			new_state = CAN_STATE_ERROR_ACTIVE;
> -		else
> -			new_state = CAN_STATE_ERROR_WARNING;
> -	} else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE))
> +		tx_state = unlikely(reg_esr & FLEXCAN_ESR_TX_WRN) ?
> +			   CAN_STATE_ERROR_WARNING : CAN_STATE_ERROR_ACTIVE;
> +		rx_state = unlikely(reg_esr & FLEXCAN_ESR_RX_WRN) ?
> +			   CAN_STATE_ERROR_WARNING : CAN_STATE_ERROR_ACTIVE;
> +		new_state = max(tx_state, rx_state);
> +	} else if (unlikely(flt == FLEXCAN_ESR_FLT_CONF_PASSIVE)) {
> +		__flexcan_get_berr_counter(dev, &bec);
>  		new_state = CAN_STATE_ERROR_PASSIVE;
> -	else
> +		rx_state = bec.rxerr >= bec.txerr ? new_state : 0;
> +		tx_state = bec.rxerr <= bec.txerr ? new_state : 0;
> +	} else {
>  		new_state = CAN_STATE_BUS_OFF;
> +	}
>  
>  	/* state hasn't changed */
>  	if (likely(new_state == priv->can.state))
> @@ -678,8 +610,11 @@ static int flexcan_poll_state(struct net_device *dev, u32 reg_esr)
>  	if (unlikely(!skb))
>  		return 0;
>  
> -	do_state(dev, cf, new_state);
> -	priv->can.state = new_state;

On SJA1000 and MSCAN you use:

	if (new_state != priv->can.state) ...

> +	can_change_state(dev, cf, new_state, tx_state, rx_state);
> +
> +	if (unlikely(new_state == CAN_STATE_BUS_OFF))
> +		can_bus_off(dev);
> +
>  	netif_receive_skb(skb);
>  
>  	dev->stats.rx_packets++;
>