netdev - Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 17 Mar 2017 18:04:42 +0100
From:   Wolfgang Grandegger <wg@...ndegger.com>
To:     Akshay Bhat <akshay.bhat@...esys.com>,
        Akshay Bhat <nodeax@...il.com>
Cc:     mkl@...gutronix.de, linux-can@...r.kernel.org,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] can: spi: hi311x: Add Holt HI-311x CAN driver

Hi Akshay,

Am 17.03.2017 um 17:00 schrieb Akshay Bhat:
> Hi Wolfgang,
>
> On 03/17/2017 03:39 AM, Wolfgang Grandegger wrote:
>> Hello Akshay,
>>
>> Am 16.03.2017 um 23:29 schrieb Akshay Bhat:
>>> Hi Wolfgang,
>>>
>>> On 03/16/2017 04:02 PM, Wolfgang Grandegger wrote:
>>>>
>>>> Looks much better now! There are message for state changes to error
>>>> warning and passive. Just the following message is not correct:
>>>>
>>>>  (000.200824)  can0  20000004   [8]  00 40 00 00 00 00 5F 19
>>>> ERRORFRAME
>>>>     controller-problem{}
>>>>     error-counter-tx-rx{{95}{25}}
>>>>
>>>> Sorry, forgot to mention... the function can_change_state() [1]
>>>> should be used for that purpose, if possible. It fixes the issue
>>>> above as well.
>>>>
>>>
>>> The updated driver (the one used to create the above log) is using
>>> can_change_state() function. data[1] 40 corresponds to
>>> CAN_ERR_CRTL_ACTIVE, so looks correct? Could it be that the can-utils I
>>> am using is old and not reporting state change?
>>
>> Hm, yes. The raw data looks correct. You could download and build a
>> recent version from "https://github.com/linux-can/can-utils" to check.
>> It could also be a bug.
>>
>
> Turned out to be a old version of can-utils. Using the above git tree
> reports the flag.
>
>  (000.200308)  can0  20000004   [8]  00 40 00 00 00 00 5F 00   ERRORFRAME
>         controller-problem{back-to-error-active}
>         error-counter-tx-rx{{95}{0}}
>
>>> Enabling BUSOFF/ERRP/ERRW bits in STATFE did not generate any interrupts
>>> on the INT pin. Should we make it a requirement that both INT and STAT
>>> pins need to be connected in hardware for the driver to do the error
>>> reporting?
>>
>> As I said, it's the better solution, especially if interrupt flooding
>> does harm. How does your system behave when bus errors come in due to no
>> cable connected?
>>
>
> I did not see any issues on the system with the cable disconnected. In
> my particular setup with the cable disconnected the system goes to
> tx-error-passive and does not get any further interrupts until a state
> change occurs.

Hm, that's unusual. Cable disconnected and then send a message:

$ grep /proc/interrupts; sleep 10; /proc/interrupts

should make things clear. But maybe it's a clever chip and it does stop 
sending error messages if the error counter does not change any more. 
After bus-off, the chip is quiet, of course. Should have a closer look 
to the CAN standard.

>> So far using NAPI was mandatory. There is the problem of out-of-order
>> message reception if handled in the isr on multi processor systems.
>> Marc, what is the current policy?
>>
>
> Since this is a SPI based CAN, I am wary for any additional latencies
> NAPI might introduce. The RX handling is being done at the very
> beginning of the ISR for this reason.
>
> Can we go ahead with the existing implementation and re-visit this at a
> later time?

Likely yes, as Marc has already reviewed the driver once.

BTW: what system board/processor are you using?

> Thanks again for all your help in reviewing/improving the driver :)

You are welcome!

Wolfgang.