Re: c_can: (newbie) high system load when frame not acked?

[Tom Evans <tom_usenet@optusnet.com.au>]

On 14/01/15 20:55, Andri Yngvason wrote:
> Quoting Tom Evans (2015-01-14 01:35:46)
>> On 14/01/15 02:32, Andri Yngvason wrote:
>>> Quoting Viktor Babrian (2015-01-13 15:10:29)
>>>
>>> Note: The FlexCAN driver does actually respect berr-reporting for i.MX6.
>>> However, it does not respect it for some older chips.
>>
>> The flexcan.c driver behaves differently depending on whether it has
>> "FLEXCAN_HAS_BROKEN_ERR_STATE" set (on for MX25, MX35, MX53 and off for MX28
>> and MX6). I don't think this distinction actually helps, or improves matters
>> enough to be worth the CPU loading this causes in these disconnected/solo CAN
>> states.
>>
> I agree.
>
>> ...
>> Does "FLEXCAN_HAS_BROKEN_ERR_STATE" do anything useful that I've missed? If
>> not then that code should be removed. I've done that in the old (Linux 3.4)
>> version of the driver I'm using.
>>
> The first step towards making that change is posting patches. ;)

Of course. But I'm working on a Linux 3.4 code base that was "frozen" about 15 
linux versions ago. I can only generate AND TEST patches for that version. 
There have been at least 19 mainstream releases/changes to that code since 
that time.

I could generate an untested patch for a later version, but I'd rather not. 
I'd rather someone who can test the code do this on the current codebase.

I would propose one of:

1 - Remove ".features = FLEXCAN_HAS_BROKEN_ERR_STATE," from all structures 
referencing all chips, like "fsl_p1010_devtype_data". That would be misleading 
though and would make the code more confusing than it is (and it is bad enough 
at the moment).

2 - Leave "FLEXCAN_HAS_BROKEN_ERR_STATE" for the chips where the WARNING 
interrupt doesn't work, but don't DO anything as a consequence. That means 
changing the following to ONLY set the error mask on 
"CAN_CTRLMODE_BERR_REPORTING" - removing the condition at line 942:

937         /*
938          * enable the "error interrupt" (FLEXCAN_CTRL_ERR_MSK),
939          * on most Flexcan cores, too. Otherwise we don't get
940          * any error warning or passive interrupts.
941          */
942         if (priv->devtype_data->features & FLEXCAN_HAS_BROKEN_ERR_STATE ||
943             priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
944                 reg_ctrl |= FLEXCAN_CTRL_ERR_MSK;
945         else
946                 reg_ctrl &= ~FLEXCAN_CTRL_ERR_MSK;

The comment in the above is very misleading. "Error Interrupts" have nothing 
to do with "FLEXCAN_HAS_BROKEN_ERR_STATE". Neither does "passive" as no 
FlexCAN cores can generate "passive interrupts". Some cores can generate 
"warning", but that's a useless interrupt as it doesn't do anything useful.

It should only say the error interrupts are enabled for 
CAN_CTRLMODE_BERR_REPORTING. For "historical interest" it could say that 
"FLEXCAN_HAS_BROKEN_ERR_STATE" used to set the error interrupts, but it causes 
interrupt flooding and can't fix the "state tracking" anyway.

Tom Evans