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

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

[Andri Yngvason]

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

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

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)
+{
+	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__);
+		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++;
+		break;
+	default:
+		netdev_warn(dev, "%s: %d is not a state", __func__, new_state);
+		break;
+	};
+}
+
+static int can_txstate_to_frame(struct net_device *dev, enum can_state 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__);
+		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)
+{
+	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;
+	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__);
+
+	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;
+}
+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 */
-- 
1.9.1

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

[Wolfgang Grandegger]

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 */
> 

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

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-11-25 20:55:22)
> 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.
>
Roger.
> 
> > 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.
> 
OK.
>
> > 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/ ?
> 
Yeah, that makes sense.
>
> 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.
> 
This is actually a warning for the developer. It's means to tell him he's doing
something seriously wrong. Maybe this should be an error then?
>
> > +             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().
>
Ooops.
> 
> > +             break;
> > +     default:
> > +             netdev_warn(dev, "%s: %d is not a state", __func__, new_state);
> 
> "%d is not a valid state" ?
>
Same as above. It's meant to tell the developer that he's putting nonsense into
state.
> 
> > +             break;
> > +     };
> > +}
> > +
> > +static int can_txstate_to_frame(struct net_device *dev, enum can_state state)
> 
> s/txstate/tx_state/ ?
>
OK.
> 
> > +{
> > +     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/
>
OK.
> 
> > +{
> > +     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?
> 
Same as above. If I were doing this in user-space, this would be an assert.
Should we maybe just ignore such things? In that case, passing new_state as an
argument is redundant because new_state = max(rx_state, tx_state).
>
> > +
> > +     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 */
> >

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

[Wolfgang Grandegger]

On Wed, 26 Nov 2014 10:26:59 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Quoting Wolfgang Grandegger (2014-11-25 20:55:22)
>> 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.
>>
> Roger.
>> 
>> > 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.
>> 
> OK.
>>
>> > 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/ ?
>> 
> Yeah, that makes sense.
>>
>> 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.
>> 
> This is actually a warning for the developer. It's means to tell him
he's
> doing
> something seriously wrong. Maybe this should be an error then?
>>
>> > +             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().
>>
> Ooops.

I think it should be moved out of can_bus_off(). This requires a separate
patch moving the line back to the drivers using can_bus_off().

Wolfgang.

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

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-11-26 11:32:56)
> On Wed, 26 Nov 2014 10:26:59 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:
> > Quoting Wolfgang Grandegger (2014-11-25 20:55:22)
> >> On 09/26/2014 07:19 PM, Andri Yngvason wrote:
...
> >> > +     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.
> >> 
> > This is actually a warning for the developer. It's means to tell him
> he's
> > doing
> > something seriously wrong. Maybe this should be an error then?
>
Any thoughts on this? Should I drop it or make a bigger bang?
>
> >>
> >> > +             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().
> >>
> > Ooops.
> 
> I think it should be moved out of can_bus_off(). This requires a separate
> patch moving the line back to the drivers using can_bus_off().
> 
Thinking the same thing. We can do that later, though; right?

--
Andri

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

[Wolfgang Grandegger]

On Wed, 26 Nov 2014 14:12:25 +0000, Andri Yngvason
<andri.yngvason@marel.com> wrote:
> Quoting Wolfgang Grandegger (2014-11-26 11:32:56)
>> On Wed, 26 Nov 2014 10:26:59 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>> > Quoting Wolfgang Grandegger (2014-11-25 20:55:22)
>> >> On 09/26/2014 07:19 PM, Andri Yngvason wrote:
> ...
>> >> > +     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?",

This indicates a state change active to active, right? The it's definitely
an error. Can it happen (code wise)?

>> >> > +                         __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.
>> >> 
>> > This is actually a warning for the developer. It's means to tell him
>> he's
>> > doing
>> > something seriously wrong. Maybe this should be an error then?
>>
> Any thoughts on this? Should I drop it or make a bigger bang?

To realize malfunctioning it's better to have an error message, I think.
Something like "invalid state change to error active detected"

>>
>> >>
>> >> > +             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().
>> >>
>> > Ooops.
>> 
>> I think it should be moved out of can_bus_off(). This requires a
separate
>> patch moving the line back to the drivers using can_bus_off().
>> 
> Thinking the same thing. We can do that later, though; right?

It should be addressed by this patch series because above is the right
place to increment can_stats.bus_off. What do you think naming the
function "can_change_state_and_update_stats()"? This would make pretty
clear what it does.

Wolfgang.

Wolfgang.

> --
> Andri

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

[Andri Yngvason]

Quoting Wolfgang Grandegger (2014-11-26 14:55:10)
> On Wed, 26 Nov 2014 14:12:25 +0000, Andri Yngvason
> <andri.yngvason@marel.com> wrote:
> > Quoting Wolfgang Grandegger (2014-11-26 11:32:56)
> >> On Wed, 26 Nov 2014 10:26:59 +0000, Andri Yngvason
> >> <andri.yngvason@marel.com> wrote:
> >> > Quoting Wolfgang Grandegger (2014-11-25 20:55:22)
> >> >> On 09/26/2014 07:19 PM, Andri Yngvason wrote:
> > ...
> >> >> > +     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?",
> 
> This indicates a state change active to active, right? The it's definitely
> an error. Can it happen (code wise)?
> 
Well, no, I'll just remove this.
>
> >> >> > +                         __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.
> >> >> 
> >> > This is actually a warning for the developer. It's means to tell him
> >> he's
> >> > doing
> >> > something seriously wrong. Maybe this should be an error then?
> >>
> > Any thoughts on this? Should I drop it or make a bigger bang?
> 
> To realize malfunctioning it's better to have an error message, I think.
> Something like "invalid state change to error active detected"
>
Agreed.
> 
> >>
> >> >>
> >> >> > +             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().
> >> >>
> >> > Ooops.
> >> 
> >> I think it should be moved out of can_bus_off(). This requires a
> separate
> >> patch moving the line back to the drivers using can_bus_off().
> >> 
> > Thinking the same thing. We can do that later, though; right?
> 
> It should be addressed by this patch series because above is the right
> place to increment can_stats.bus_off. What do you think naming the
> function "can_change_state_and_update_stats()"? This would make pretty
> clear what it does.
> 
I think that functions shouldn't actually do more than one thing and that if you
put an "and" in the function name, that's a really good indicator that the
function is doing more than it should.

The rationale here is that you shouldn't have to read the function to figure out
what it actually does when reading the code that uses it.

Also, we would actually have to call the function
"can_change_state_and_update_stats_and_compose_error_state_error_frame()"
which seems rather absurd.

"can_change_state" actually does three things, two of which are not changing the
state. We could change it to "can_process_state" which is more ambiguous, but
less misleading. Or maybe "can_do_state" or "can_feed_state".

I'm leaning towards splitting it up, but I'm fine with one function also if you
like that better.

Maybe someone else would like to chime in on this? Marc?

--
Andri

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

[Wolfgang Grandegger]

On 11/26/2014 04:38 PM, Andri Yngvason wrote:
> Quoting Wolfgang Grandegger (2014-11-26 14:55:10)
>> On Wed, 26 Nov 2014 14:12:25 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>>> Quoting Wolfgang Grandegger (2014-11-26 11:32:56)
>>>> On Wed, 26 Nov 2014 10:26:59 +0000, Andri Yngvason
>>>> <andri.yngvason@marel.com> wrote:
>>>>> Quoting Wolfgang Grandegger (2014-11-25 20:55:22)
>>>>>> On 09/26/2014 07:19 PM, Andri Yngvason wrote:
>>> ...
>>>>>>> +     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?",
>>
>> This indicates a state change active to active, right? The it's definitely
>> an error. Can it happen (code wise)?
>>
> Well, no, I'll just remove this.
>>
>>>>>>> +                         __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.
>>>>>>
>>>>> This is actually a warning for the developer. It's means to tell him
>>>> he's
>>>>> doing
>>>>> something seriously wrong. Maybe this should be an error then?
>>>>
>>> Any thoughts on this? Should I drop it or make a bigger bang?
>>
>> To realize malfunctioning it's better to have an error message, I think.
>> Something like "invalid state change to error active detected"
>>
> Agreed.
>>
>>>>
>>>>>>
>>>>>>> +             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().
>>>>>>
>>>>> Ooops.
>>>>
>>>> I think it should be moved out of can_bus_off(). This requires a
>> separate
>>>> patch moving the line back to the drivers using can_bus_off().
>>>>
>>> Thinking the same thing. We can do that later, though; right?
>>
>> It should be addressed by this patch series because above is the right
>> place to increment can_stats.bus_off. What do you think naming the
>> function "can_change_state_and_update_stats()"? This would make pretty
>> clear what it does.
>>
> I think that functions shouldn't actually do more than one thing and that if you
> put an "and" in the function name, that's a really good indicator that the
> function is doing more than it should.
> 
> The rationale here is that you shouldn't have to read the function to figure out
> what it actually does when reading the code that uses it.
> 
> Also, we would actually have to call the function
> "can_change_state_and_update_stats_and_compose_error_state_error_frame()"
> which seems rather absurd.
> 
> "can_change_state" actually does three things, two of which are not changing the
> state. We could change it to "can_process_state" which is more ambiguous, but
> less misleading. Or maybe "can_do_state" or "can_feed_state".
> 
> I'm leaning towards splitting it up, but I'm fine with one function also if you
> like that better.

I'm fine with can_change_state().

Wolfgang.