Re: Inconsistent error state transition error frames

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

On þri 9.sep 2014 18:08, Oliver Hartkopp wrote:
>
> On 09.09.2014 17:41, Wolfgang Grandegger wrote:
>> On Tue, 9 Sep 2014 14:49:58 +0000, Andri Yngvason
>> <andri.yngvason@marel.com> wrote:
>
>>> It seems that 'data' is fully occupied, although 'data[5..7]' isn't
>>> reserved for anything specific but rather "controller specific
>>> additional information". Could we maybe put the version into data 5, 6
>> or
>>> 7?
>> data[6..7] is used for the tx and rx error counts.
>>   
> Oh yes. This is currently not documented in error.h :-(
>
> What about adding some defines for it, like this:
>
> diff --git a/include/uapi/linux/can/error.h b/include/uapi/linux/can/error.h
> index c247446..c290f30 100644
> --- a/include/uapi/linux/can/error.h
> +++ b/include/uapi/linux/can/error.h
> @@ -58,10 +58,12 @@
>   #define CAN_ERR_RESTARTED    0x00000100U /* controller restarted */
>   
>   /* arbitration lost in bit ... / data[0] */
> +#define CAN_ERR_LOSTARB_BYTE 0
>   #define CAN_ERR_LOSTARB_UNSPEC   0x00 /* unspecified */
>   				      /* else bit number in bitstream */
>   
>   /* error status of CAN-controller / data[1] */
> +#define CAN_ERR_CRTL_BYTE 1
>   #define CAN_ERR_CRTL_UNSPEC      0x00 /* unspecified */
>   #define CAN_ERR_CRTL_RX_OVERFLOW 0x01 /* RX buffer overflow */
>   #define CAN_ERR_CRTL_TX_OVERFLOW 0x02 /* TX buffer overflow */
> @@ -73,6 +75,7 @@
>   				      /* the protocol-defined level of 127)  */
>   
>   /* error in CAN protocol (type) / data[2] */
> +#define CAN_ERR_PROT_BYTE 2
>   #define CAN_ERR_PROT_UNSPEC      0x00 /* unspecified */
>   #define CAN_ERR_PROT_BIT         0x01 /* single bit error */
>   #define CAN_ERR_PROT_FORM        0x02 /* frame format error */
> @@ -84,6 +87,7 @@
>   #define CAN_ERR_PROT_TX          0x80 /* error occurred on transmission */
>   
>   /* error in CAN protocol (location) / data[3] */
> +#define CAN_ERR_PROT_LOC_BYTE 3
>   #define CAN_ERR_PROT_LOC_UNSPEC  0x00 /* unspecified */
>   #define CAN_ERR_PROT_LOC_SOF     0x03 /* start of frame */
>   #define CAN_ERR_PROT_LOC_ID28_21 0x02 /* ID bits 28 - 21 (SFF: 10 - 3) */
> @@ -106,6 +110,7 @@
>   #define CAN_ERR_PROT_LOC_INTERM  0x12 /* intermission */
>   
>   /* error status of CAN-transceiver / data[4] */
> +#define CAN_ERR_TRX_BYTE 4
>   /*                                             CANH CANL */
>   #define CAN_ERR_TRX_UNSPEC             0x00 /* 0000 0000 */
>   #define CAN_ERR_TRX_CANH_NO_WIRE       0x04 /* 0000 0100 */
> @@ -118,6 +123,10 @@
>   #define CAN_ERR_TRX_CANL_SHORT_TO_GND  0x70 /* 0111 0000 */
>   #define CAN_ERR_TRX_CANL_SHORT_TO_CANH 0x80 /* 1000 0000 */
>   
> -/* controller specific additional information / data[5..7] */
> +/* controller specific additional information / data[5] */
> +
> +/* controller error counter values / data[6..7] */
> +#define CAN_ERR_TXERR_BYTE 6
> +#define CAN_ERR_RXERR_BYTE 7
>   
>   #endif /* _UAPI_CAN_ERROR_H */
>
I'm not sure what's considered good practice in kernel code but wouldn't 
it be better to use a macro like this instead?
     #define CAN_ERR_CRTL_DATA(frame) ((frame)->data[1])
which would be used like this:
     if(CAN_ERR_CRTL_DATA(frame) & CAN_ERR_CRTL_whatever) ...

This looks a little nicer and reads better than 
"frame->data[CAN_ERR_CRTL_BYTE]".
>>> Another option is to place it into the 3 or 4 msb of the id; something
>>> like this:
>>> #define CAN_ERR_VERSION_MASK 0x1E000000U  // Note: CAN_ERR_MASK is
>>> 0x1FFFFFFFU
>>>
>>> Are 15 possible versions enough (or even too much)?
>> I would prefer an unsued field otherwise old can-util tools will report
>> rubbish. data[5] sounds good. But we should add a version number only
>> if we really need it.
> Yes.
>
> As the current situation needs a re-work and is somehow broken I wonder if
> the current implementation is really used widely by that many people ?!?
>
> As you introduce a new bit we should check if there is really that much
> fallout that a version info becomes necessary.
>
> Maybe be we can change the definition and only update the can-utils accordingly.
>
> Or do you see any general problem for the suggested changes?
>
I don't actually see a problem. This doesn't even work the same way for 
all protocols, so existing code that monitors error state changes can be 
considered unreliable at best.