[PATCH 1/4] Consolidate and unify state change handling

[Andri Yngvason]

Signed-off-by: Andri Yngvason <***@marel.com>
---
drivers/net/can/dev.c | 79
++++++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 10 ++++++
include/uapi/linux/can/error.h | 1 +
3 files changed, 90 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 02492d2..d238b68 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -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
*
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 6992afc..9ef20fe 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -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);
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 */
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Andri Yngvason]

Post by Andri Yngvason
+ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Andri Yngvason]

Post by Andri Yngvason

Post by Andri Yngvason
+ 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

--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[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.

Post by Andri Yngvason
---
drivers/net/can/dev.c | 79
++++++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 10 ++++++
include/uapi/linux/can/error.h | 1 +
3 files changed, 90 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 02492d2..d238b68 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -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) {
+ return CAN_ERR_CRTL_TX_WARNING;
+ return CAN_ERR_CRTL_RX_WARNING;
+ 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) {
+ return CAN_ERR_CRTL_TX_PASSIVE;
+ return CAN_ERR_CRTL_RX_PASSIVE;
+ 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.

Post by Andri Yngvason
+{
+ 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) {
+ if (state > priv->state)
+ priv->can_stats.error_warning++;
+ cf->data[1] = can_make_state_warning(err_dir);
+ break;
+
+ if (state > priv->state)
+ priv->can_stats.error_passive++;
+ cf->data[1] = can_make_state_passive(err_dir);
+ break;
+
+ cf->data[1] = CAN_ERR_CRTL_ACTIVE;
+ break;
+
+ priv->can_stats.bus_off++;
+ cf->can_id |= CAN_ERR_BUSOFF;
+ break;
+

A netdev_warn would be nice here.

Post by Andri Yngvason
+ break;
+ };
+
+ priv->state = state;
+}
+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..9ef20fe 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -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.

Post by Andri Yngvason
/*
* 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);
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 */
--

Wolfgang.



--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Andri Yngvason]

Please add a brief patch description. E.g. what is it good for and wh=

at

does it improve.

I'll do that. Thanks for pointing this out.

Post by Andri Yngvason
...
+
+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".

There are controllers that do not expose the error counters but do expo=
se
the individual states of the error counters (e.g. mscan); or maybe rath=
er
which state changed? What I proposed in my reply to [PATCH 2/4] would
solve this.

Also s/state/new_state/ would improve readability.

Yes.

Post by Andri Yngvason
+{
+ struct can_priv *priv =3D netdev_priv(dev);
+
+ if (unlikely(state =3D=3D priv->state)) {
+ netdev_warn(dev, "%s: oops, state did not change", __func__=

);

Post by Andri Yngvason
+ return;
+ }
+
+ if (state !=3D CAN_STATE_BUS_OFF)
+ cf->can_id |=3D CAN_ERR_CRTL;
+
+ switch (state) {
+ if (state > priv->state)
+ priv->can_stats.error_warning++;
+ cf->data[1] =3D can_make_state_warning(err_dir);
+ break;
+
+ if (state > priv->state)
+ priv->can_stats.error_passive++;
+ cf->data[1] =3D can_make_state_passive(err_dir);
+ break;
+
+ cf->data[1] =3D CAN_ERR_CRTL_ACTIVE;
+ break;
+
+ priv->can_stats.bus_off++;
+ cf->can_id |=3D CAN_ERR_BUSOFF;
+ break;
+

A netdev_warn would be nice here.

Yes, that would probably we wise in case someone accidentally passes=20
something
that isn't really a state.

Andri
--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html