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]

Message-ID: <0362316b-ef76-1733-dbfc-8b644f758db9@timesys.com>
Date:   Fri, 17 Mar 2017 12:00:32 -0400
From:   Akshay Bhat <akshay.bhat@...esys.com>
To:     Wolfgang Grandegger <wg@...ndegger.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 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.

> 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?

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

Akshay

> Wolfgang.