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

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

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