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

[Wolfgang Grandegger <wg@grandegger.com>]

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.