Hello,
On 02/18/2011 12:06 AM, Kurt Van Dijck wrote:
> On Thu, Feb 17, 2011 at 08:17:41PM +0100, Wolfgang Grandegger wrote:
>> On 02/17/2011 12:05 AM, Kurt Van Dijck wrote:
>>> On Wed, Feb 16, 2011 at 09:31:30PM +0100, Oliver Hartkopp wrote:
>>>> On 16.02.2011 21:04, Wolfgang Grandegger wrote:
>>>>> On 02/16/2011 08:00 PM, Oliver Hartkopp wrote:
>>>>>> On 16.02.2011 09:10, Wolfgang Grandegger wrote:
>>>>
>>>>
> [...]
>>>>>>> We then could or even should add a version information into the error
>>>>>>> message. I think data[5] could be used for that purpose.
>>>>>>
>>>>>> It's like having the CAN core version information where Dave was
>>>>>> complaining
>>>>>> when it gone Mainline. Who should check version numbers and maintain all
>>>>>> this?
>>>>>
>>>>> This would allow users already using the error flags to easily migrate
>>>>> to the new and changed error types. My idea here is:
>>>>>
>>>>> cf->data[5] = CAN_ERR_VERSION;
>>>>>
>>>>> Also candump could use it to avoid displaying rubbish with old kernels.
>>> IMO it's not so good put the version in a 'hot path'.
>>
>> What do you mean with hot path.
> 'hot path' == every CAN error frame will carry the version info.
> Opposed to uname() where the version is transferred once.
>
> Or, in other words, the version isn't going to change, but is transferred
> every time...
We always send all 8 bytes and CAN_ERR_VERSION would not change that.
>> It just adds a version dependency to the
>> error message. So it's always clear what formatting has been used.
> getsockopt() ?
>>
>>>> Hm - maybe then it's better to retrieve the Kernel version with uname(2)
>>>> ...
>>>>
>>>>> I started writing the patch and realized that we need some more
>>>>> clarification and cleanup. Next point is how we increment the
>>>>> stats->[r|t]x_error counters? It's not done consistently and it's not
>>>>> even possible to assign the direction of bus errors properly. Have a
>>>>> look to the drivers to realizes the mess. Should we do that at all? We
>>>>> have our own stats of CAN errors:
>>>>>
>>>>> struct can_device_stats {
>>>>> __u32 bus_error; /* Bus errors */
>>>>> __u32 error_warning; /* Changes to error warning state */
>>>>> __u32 error_passive; /* Changes to error passive state */
>>>>> __u32 bus_off; /* Changes to bus off state */
>>>>> __u32 arbitration_lost; /* Arbitration lost errors */
>>>>> __u32 restarts; /* CAN controller re-starts */
>>>>> };
>>>>>
>>>>> I tend to drop touching the network stats completely. What do you and
>>>>> others think.
>>>>
>>>> Using the common network stats would have the advantage, that you can
>>>> easily
>>>> see problems with standard tools like 'ifconfig' or 'cat /proc/net/dev'
>>>
>>> After dropping network stats, some other stats mechanism will appear with
>>> even less defined semantics, since some kind of stats are usefull (we even
>>> have some extra stats for CAN errors!).
>>> It is a bit messy now, I agree.
>>
>> I thin mantaining and incementing too similar stats is overkill.
>> Also, we have difficulties to map the CAN errors to the netdev errors:
>>
>> http://lxr.linux.no/#linux+v2.6.37/include/linux/netdevice.h#L173
>>
> Do you question the stats like 'bus_error', or do you question
> having them seperate?
> I see value in keeping such stats ...
I want to drop incrementing the netdev stats errors. The CAN stats
should be kept, of course. Currently, we increment both stats for bus
errors, e.g.:
can_stats->bus_errors++;
stats->rx_errors++;
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
