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

[Wolfgang Grandegger <wg@grandegger.com>]

On 09/26/2014 07:19 PM, Andri Yngvason wrote:
> The handling of can error states is different between platforms.
> This is an attempt to correct that problem.
> 
> I've moved this handling into a generic function for changing the
> error state. This ensures that error state changes are handled
> the same way everywhere (where this function is used).

I think it's also important to note that now also *decreasing* error
states are reported.

> Changes made since last proposal:
> can: dev: remove can_errcnt_to_state
> can: dev: reduce nesting in can_change_state

Please move the changes after the "---" line below.

> Signed-off-by: Andri Yngvason <andri.yngvason@marel.com>
> ---
>  drivers/net/can/dev.c          | 94 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/can/dev.h        |  4 ++
>  include/uapi/linux/can/error.h |  1 +
>  3 files changed, 99 insertions(+)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 02492d2..a10b6ab 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -273,6 +273,100 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
>  	return err;
>  }
>  
> +static void can_update_error_counters(struct net_device *dev,
> +				      enum can_state new_state)

s/can_update_error_counters/can_update_error_stats/ ?

Error counters are usually txerr/rxerr.

> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	if (new_state <= priv->state)
> +		return;
> +
> +	switch (new_state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		netdev_warn(dev, "%s: did we come from a state less than error-active?",
> +			    __func__);

Please remove __func__ here and below and use a more meaningful warning
message. Such messages should make sense to non-experts as well...
at least a little bit.

> +		break;
> +	case CAN_STATE_ERROR_WARNING:
> +		priv->can_stats.error_warning++;
> +		break;
> +	case CAN_STATE_ERROR_PASSIVE:
> +		priv->can_stats.error_passive++;
> +		break;
> +	case CAN_STATE_BUS_OFF:
> +		priv->can_stats.bus_off++;

Be careful here. This counter will also be incremented in can_bus_off().

> +		break;
> +	default:
> +		netdev_warn(dev, "%s: %d is not a state", __func__, new_state);

"%d is not a valid state" ?

> +		break;
> +	};
> +}
> +
> +static int can_txstate_to_frame(struct net_device *dev, enum can_state state)

s/txstate/tx_state/ ?

> +{
> +	switch (state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		return CAN_ERR_CRTL_ACTIVE;
> +	case CAN_STATE_ERROR_WARNING:
> +		return CAN_ERR_CRTL_TX_WARNING;
> +	case CAN_STATE_ERROR_PASSIVE:
> +		return CAN_ERR_CRTL_TX_PASSIVE;
> +	case CAN_STATE_BUS_OFF:
> +		netdev_warn(dev, "%s: bus-off is not handled here", __func__);

Then we may silently ignore it?

> +		return 0;
> +	default:
> +		netdev_warn(dev, "%s: %d is not a state", __func__, state);
> +		return 0;
> +	}
> +}
> +
> +static int can_rxstate_to_frame(struct net_device *dev, enum can_state state)

s/rxstate/rx_state/

> +{
> +	switch (state) {
> +	case CAN_STATE_ERROR_ACTIVE:
> +		return CAN_ERR_CRTL_ACTIVE;
> +	case CAN_STATE_ERROR_WARNING:
> +		return CAN_ERR_CRTL_RX_WARNING;
> +	case CAN_STATE_ERROR_PASSIVE:
> +		return CAN_ERR_CRTL_RX_PASSIVE;
> +	case CAN_STATE_BUS_OFF:
> +		netdev_warn(dev, "%s: bus-off is not handled here", __func__);
> +		return 0;

See above.

> +	default:
> +		netdev_warn(dev, "%s: %d is not a state", __func__, state);
> +		return 0;
> +	}
> +}
> +
> +void can_change_state(struct net_device *dev, struct can_frame *cf,
> +		      enum can_state new_state, enum can_state tx_state,
> +		      enum can_state rx_state)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	if (unlikely(new_state == priv->state)) {
> +		netdev_warn(dev, "%s: oops, state did not change", __func__);
> +		return;
> +	}
> +
> +	can_update_error_counters(dev, new_state);
> +	priv->state = new_state;
> +
> +	if (unlikely(new_state == CAN_STATE_BUS_OFF)) {
> +		cf->can_id |= CAN_ERR_BUSOFF;
> +		return;
> +	}
> +
> +	if (unlikely(tx_state != new_state && rx_state != new_state))
> +		netdev_warn(dev, "%s: neither rx nor tx state match new state", __func__);

Is this worth a warning or is it even an error?

> +
> +	cf->can_id |= CAN_ERR_CRTL;
> +	cf->data[1] |= tx_state >= rx_state ?
> +		       can_txstate_to_frame(dev, tx_state) : 0;
> +	cf->data[1] |= tx_state <= rx_state ?
> +		       can_rxstate_to_frame(dev, rx_state) : 0;

OK, I can live with that solution.

> +}
> +EXPORT_SYMBOL_GPL(can_change_state);
> +
>  /*
>   * Local echo of CAN messages
>   *
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 6992afc..1902bff 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -121,6 +121,10 @@ void unregister_candev(struct net_device *dev);
>  int can_restart_now(struct net_device *dev);
>  void can_bus_off(struct net_device *dev);
>  
> +void can_change_state(struct net_device *dev, struct can_frame *cf,
> +		      enum can_state new_state, enum can_state tx_state,
> +		      enum can_state rx_state);
> +
>  void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  		      unsigned int idx);
>  unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
> diff --git a/include/uapi/linux/can/error.h b/include/uapi/linux/can/error.h
> index c247446..1c508be 100644
> --- a/include/uapi/linux/can/error.h
> +++ b/include/uapi/linux/can/error.h
> @@ -71,6 +71,7 @@
>  #define CAN_ERR_CRTL_TX_PASSIVE  0x20 /* reached error passive status TX */
>  				      /* (at least one error counter exceeds */
>  				      /* the protocol-defined level of 127)  */
> +#define CAN_ERR_CRTL_ACTIVE      0x40 /* recovered to error active state */
>  
>  /* error in CAN protocol (type) / data[2] */
>  #define CAN_ERR_PROT_UNSPEC      0x00 /* unspecified */
>