Re: flexcan missing error state transitions

[朱燚 <zhuyi882004@126.com>]

Hi Andri,

> That sounds rather shaky. Wouldn't it be more robust to have the timer be
> periodic? Besides, over-sampling is preferable in order to avoid missed
> transitions, right? See diff below.
I can understand why you feel shaky, but I think use periodic timer will not
do meaningful work here - just polling state which do not change.

The reason is flexcan_irq_state() will be called upon any interrupt, that
already guarantees state is up-to-date if there is traffic on the bus, which
is the trigger of any possible state transitions. In this case, any kind of
polling is not required (however, this patch will still do once extra polling,
which I'd like to avoid as well, but seems no easy solution). Extra polling
will get the same state as before if there is no more traffic, and if the
traffic is ongoing, the interrupt will be fired.

The only case above approach cannot handle is there is only singled out node
on the bus, where the traffic cannot continue. Although the core will perform
auto-retransmission, but it will not generate flood of error interrupts,
instead only generate an ACK error (/+WARN) interrupt, where the FLT_CONF is
not up-to-date yet when the flexcan_irq_state() been called, and there is no
trigger to update the state again when the register is changed. That's the
point of this patch to compensate this trigger by poll once at the right time.
Extra polling will get the same state too if the bus is still open, and if the
bus is recovered, the interrupt will be fired again.

Regarding missed transitions, yes, if user chose a too short delay, then he
will miss the error passive transition, that's why the delay is configurable
via DT.

> > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> ...
> > +static void flexcan_update_state_timeout(unsigned long data)
> > +{
> > +	struct net_device *dev =3D (struct net_device*)data;
> > +	struct flexcan_priv *priv =3D netdev_priv(dev);
> > +	struct flexcan_regs __iomem *regs =3D priv->regs;
> > +	u32 reg_esr;
> > +
> > +	/* update state atomically */
> > +	disable_irq(dev->irq);
> > +	reg_esr = flexcan_read(&regs->esr);
> > +	flexcan_irq_state(dev, reg_esr);
> You could restart the timer here if the error state is not error-active. Then
> you would not have the problem of having to choose an interval so carefully.
Yes, you're right. But I think it is good to think carefully which delay is
really necessary than just drop a randomly number here. With periodic timer,
if very short delay assigned, then the performance will be hit, if reasonable
delay assigned, then unnecessary polling will be done.
> > +	enable_irq(dev->irq);
> > +}
> ...

I understand that use periodic timer is a more generic approach which even
possible to replace the interrupt driven on broken core (if want to), but I
want to avoid unnecessary polling as much as possible (and still get the
correct state transitions).

What do you think?

Best regards
Yi