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

[Andri Yngvason <andri.yngvason@marel.com>]

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