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

[Wolfgang Grandegger]

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.