Consolidate and unify state change handling
PATCH 1/4 Consolidate and unify state change handling
--- drivers/net/can/dev.c | 79 ++++++++++++++++++++++++++++++++++++++++++ include/linux/can/dev.h | 10 ++++++ include/uapi/linux/can/error.h | 1 + 3 files changed, 90 insertions(+)
drivers/net/can/dev.c +79 -0
@@ -273,6 +273,85 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,return err; } +enum can_err_dir can_get_err_dir (unsigned int txerr,unsigned int rxerr) +{ +if (txerr > rxerr) +return CAN_ERR_DIR_TX ; + +if (rxerr < rxerr) +return CAN_ERR_DIR_RX ; + +return CAN_ERR_DIR_UNKNOWN ; +} +EXPORT_SYMBOL_GPL (can_get_err_dir); + +static int can_make_state_warning (enum can_err_dir err_dir) +{ +switch (err_dir) { +case CAN_ERR_DIR_TX : +return CAN_ERR_CRTL_TX_WARNING ; +case CAN_ERR_DIR_RX : +return CAN_ERR_CRTL_RX_WARNING ; +default : +return CAN_ERR_CRTL_TX_WARNING |CAN_ERR_CRTL_RX_WARNING ; + } +} + +static int can_make_state_passive (enum can_err_dir err_dir) +{ +switch (err_dir) { +case CAN_ERR_DIR_TX : +return CAN_ERR_CRTL_TX_PASSIVE ; +case CAN_ERR_DIR_RX : +return CAN_ERR_CRTL_RX_PASSIVE ; +default : +return CAN_ERR_CRTL_TX_PASSIVE |CAN_ERR_CRTL_RX_PASSIVE ; + } +} + +void can_change_state (struct net_device *dev,struct can_frame *cf, +enum can_state state,enum can_err_dir err_dir) +{ +struct can_priv *priv =netdev_priv (dev); + +if (unlikely (state == priv->state)) { +netdev_warn (dev,"%s: oops, state did not change" , __func__); +return ; + } + +if (state !=CAN_STATE_BUS_OFF ) + cf->can_id |=CAN_ERR_CRTL ; + +switch (state) { +case CAN_STATE_ERROR_WARNING : +if (state > priv->state) + priv->can_stats.error_warning++; + cf->data[1 ] =can_make_state_warning (err_dir); +break ; + +case CAN_STATE_ERROR_PASSIVE : +if (state > priv->state) + priv->can_stats.error_passive++; + cf->data[1 ] =can_make_state_passive (err_dir); +break ; + +case CAN_STATE_ERROR_ACTIVE : + cf->data[1 ] =CAN_ERR_CRTL_ACTIVE ; +break ; + +case CAN_STATE_BUS_OFF : + priv->can_stats.bus_off++; + cf->can_id |=CAN_ERR_BUSOFF ; +break ; + +default : +break ; + }; + + priv->state = state; +} +EXPORT_SYMBOL_GPL (can_change_state); +/* * Local echo of CAN messages *
include/linux/can/dev.h +10 -0
@@ -27,6 +27,12 @@ enum can_mode { CAN_MODE_SLEEP }; +enum can_err_dir { +CAN_ERR_DIR_UNKNOWN =0 , +CAN_ERR_DIR_RX , +CAN_ERR_DIR_TX +}; +/* * CAN common private data */
@@ -121,6 +127,10 @@ void unregister_candev(struct net_device *dev);int can_restart_now (struct net_device *dev);void can_bus_off (struct net_device *dev); +enum can_err_dir can_get_err_dir (unsigned int txerr,unsigned int rxerr); +void can_change_state (struct net_device *dev,struct can_frame *cf, +enum can_state state,enum can_err_dir err_dir); +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);
include/uapi/linux/can/error.h +0 -0
@@ -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 */
A message that is not in the archive (sent off-list or not yet received): CAHuoermUnpToi9TzMDL9+h_LD4yQtuhdG6M=yhs7Q7vFLjNvKA@mail.gmail.com
Quoted 4 lines · Paul Lever
drivers/net/can/dev.c +2
-0+if (rxerr < rxerr) +return CAN_ERR_DIR_RX ;This doesn't look right. rxerr can't be less than itself.
Whoa! Nice catch. That should be "txerr < rxerr".
This error would be near to impossible to catch in tests because this means that the case where "txerr == rxerr" would never happen.
Andri
Quoted 9 lines · Andri Yngvason
Quoted 4 lines · Paul Lever
drivers/net/can/dev.c +2
-0+if (rxerr < rxerr) +return CAN_ERR_DIR_RX ;This doesn't look right. rxerr can't be less than itself.
Whoa! Nice catch. That should be "txerr < rxerr".
This error would be near to impossible to catch in tests because this means that the case where "txerr == rxerr" would never happen.
No, wait, sorry, brainfart. It's the other way around: the "txerr < rxerr" case never happens. I.e. either returns CAN_ERR_DIR_RX or CAN_ERR_DIR_RX | CAN_ERR_DIR_TX.
Andri
On 09/18/2014 06:25 PM, Andri Yngvason wrote:
Please add a brief patch description. E.g. what is it good for and what does it improve.
Quoted 45 lines
Signed-off-by Andri Yngvason <[email protected]>--- drivers/net/can/dev.c | 79 ++++++++++++++++++++++++++++++++++++++++++ include/linux/can/dev.h | 10 ++++++ include/uapi/linux/can/error.h | 1 + 3 files changed, 90 insertions(+)drivers/net/can/dev.c +28
-0@@ -273,6 +273,85 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt, return err; } +enum can_err_dir can_get_err_dir (unsigned int txerr,unsigned int rxerr) +{ +if (txerr > rxerr) +return CAN_ERR_DIR_TX ; + +if (rxerr < rxerr) +return CAN_ERR_DIR_RX ; + +return CAN_ERR_DIR_UNKNOWN ; +} +EXPORT_SYMBOL_GPL (can_get_err_dir); + +static int can_make_state_warning (enum can_err_dir err_dir) +{ +switch (err_dir) { +case CAN_ERR_DIR_TX : +return CAN_ERR_CRTL_TX_WARNING ; +case CAN_ERR_DIR_RX : +return CAN_ERR_CRTL_RX_WARNING ; +default : +return CAN_ERR_CRTL_TX_WARNING |CAN_ERR_CRTL_RX_WARNING ; + } +} + +static int can_make_state_passive (enum can_err_dir err_dir) +{ +switch (err_dir) { +case CAN_ERR_DIR_TX :
drivers/net/can/dev.c static int can_get_bittiming(struct net_device · line 314
Quoted 10 lines
drivers/net/can/dev.c +10
-0+return CAN_ERR_CRTL_TX_PASSIVE ; +case CAN_ERR_DIR_RX : +return CAN_ERR_CRTL_RX_PASSIVE ; +default : +return CAN_ERR_CRTL_TX_PASSIVE |CAN_ERR_CRTL_RX_PASSIVE ; + } +} + +void can_change_state (struct net_device *dev,struct can_frame *cf, +enum can_state state,enum can_err_dir err_dir)
Thinking about it... passing txerr and rxerr directly here would make the err_dir obsolete and the code more simpler. CAN controllers not supporting tx/rx error counter should pass "0, 0".
Also s/state/new_state/ would improve readability.
Quoted 24 lines
drivers/net/can/dev.c +24
-0+{ +struct can_priv *priv =netdev_priv (dev); + +if (unlikely (state == priv->state)) { +netdev_warn (dev,"%s: oops, state did not change" , __func__); +return ; + } + +if (state !=CAN_STATE_BUS_OFF ) + cf->can_id |=CAN_ERR_CRTL ; + +switch (state) { +case CAN_STATE_ERROR_WARNING : +if (state > priv->state) + priv->can_stats.error_warning++; + cf->data[1 ] =can_make_state_warning (err_dir); +break ; + +case CAN_STATE_ERROR_PASSIVE : +if (state > priv->state) + priv->can_stats.error_passive++; + cf->data[1 ] =can_make_state_passive (err_dir); +break ; +
drivers/net/can/dev.c static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt, · line 347
Quoted 10 lines
drivers/net/can/dev.c +10
-0+case CAN_STATE_ERROR_ACTIVE : + cf->data[1 ] =CAN_ERR_CRTL_ACTIVE ; +break ; + +case CAN_STATE_BUS_OFF : + priv->can_stats.bus_off++; + cf->can_id |=CAN_ERR_BUSOFF ; +break ; + +default :
A netdev_warn would be nice here.
Quoted 15 lines
drivers/net/can/dev.c +7
-0+break ; + }; + + priv->state = state; +} +EXPORT_SYMBOL_GPL (can_change_state); +/* * Local echo of CAN messages *
include/linux/can/dev.h +0
-0
include/linux/can/dev.h enum can_mode { · line 34
Quoted 8 lines
include/linux/can/dev.h +5
-0@@ -27,6 +27,12 @@ enum can_mode { CAN_MODE_SLEEP }; +enum can_err_dir { +CAN_ERR_DIR_UNKNOWN =0 , +CAN_ERR_DIR_RX , +CAN_ERR_DIR_TX +};
See my comment above.
Quoted 22 lines
include/linux/can/dev.h +4
-0/* * CAN common private data */ @@ -121,6 +127,10 @@ void unregister_candev(struct net_device *dev);int can_restart_now (struct net_device *dev);void can_bus_off (struct net_device *dev); +enum can_err_dir can_get_err_dir (unsigned int txerr,unsigned int rxerr); +void can_change_state (struct net_device *dev,struct can_frame *cf, +enum can_state state,enum can_err_dir err_dir); +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);include/uapi/linux/can/error.h +0
-0@@ -71,6 +71,7 @@#define CAN_ERR_CRTL_TX_PASSIVE 0x20/* reached error passive status TX */
Quoted 8 lines
/* (at least one error counter exceeds */ /* the protocol-defined level of 127) */+#define
CAN_ERR_CRTL_ACTIVE0x40/* recovered to error active
state *//* error in CAN protocol (type) / data[2] */
#defineCAN_ERR_PROT_UNSPEC0x00/* unspecified */
Wolfgang.
Quoted 4 lines · Wolfgang Grandegger
On 09/18/2014 06:25 PM, Andri Yngvason wrote:
Please add a brief patch description. E.g. what is it good for and what does it improve.
I'll do that. Thanks for pointing this out.
Quoted 7 lines
drivers/net/can/dev.c static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt, · line 313
Quoted 4 lines
...
drivers/net/can/dev.c +3
-0+ +void can_change_state (struct net_device *dev,struct can_frame *cf, +enum can_state state,enum can_err_dir err_dir)Thinking about it... passing txerr and rxerr directly here would make the
err_dirobsolete and the code more simpler. CAN controllers not supporting tx/rx error counter should pass "0, 0".
There are controllers that do not expose the error counters but do expose the individual states of the error counters (e.g. mscan); or maybe rather which state changed? What I proposed in my reply to [PATCH 2/4] would solve this.
Quoted 2 lines
Also s/state/new_state/ would improve readability.
Yes.
Quoted 28 lines
Quoted 28 lines
drivers/net/can/dev.c +27
-0+{ +struct can_priv *priv =netdev_priv (dev); + +if (unlikely (state == priv->state)) { +netdev_warn (dev,"%s: oops, state did not change" , __func__); +return ; + } + +if (state !=CAN_STATE_BUS_OFF ) + cf->can_id |=CAN_ERR_CRTL ; + +switch (state) { +case CAN_STATE_ERROR_WARNING : +if (state > priv->state) + priv->can_stats.error_warning++; + cf->data[1 ] =can_make_state_warning (err_dir); +break ; + +case CAN_STATE_ERROR_PASSIVE : +if (state > priv->state) + priv->can_stats.error_passive++; + cf->data[1 ] =can_make_state_passive (err_dir); +break ; + +case CAN_STATE_ERROR_ACTIVE : + cf->data[1 ] =CAN_ERR_CRTL_ACTIVE ; +break ;
Quoted 8 lines
drivers/net/can/dev.c static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt, · line 347
Quoted 7 lines
drivers/net/can/dev.c +7
-0+ +case CAN_STATE_BUS_OFF : + priv->can_stats.bus_off++; + cf->can_id |=CAN_ERR_BUSOFF ; +break ; + +default :A
netdev_warnwould be nice here.
Yes, that would probably we wise in case someone accidentally passes something that isn't really a state.
Andri