Re: Inconsistent error state transition error frames

[Wolfgang Grandegger <wg@grandegger.com>]

On Wed, 10 Sep 2014 12:29:23 +0200, Marc Kleine-Budde <mkl@pengutronix.de>
wrote:
> On 09/10/2014 12:19 PM, Andri Yngvason wrote:
>>> 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) ...
> 
> I'm not sure....but...
> 
> Make it a static inline function and give it a proper name, something
> like can_err_frame_get_ctrl(). Maybe without err_....

I'm not sure... we also need to set the value. Therefore I would
vote for:

 frame->data[CAN_ERR_TXERR_BYTE];

It does make the code more readable.

Wolfgang.