Hey all,
I ran into this same problem, and picked up the patches.
On ma, 23 jul 2018 13:36:14 +0200, Marc Kleine-Budde wrote:
> On 06/21/2018 03:20 PM, Wolfgang Grandegger wrote:
...
> >>> Looking to the code, I see a problem with reading the status register
> >>> (C_CAN_STS_REG). It should *only* be read if bit 15 of the interrupt
> >>> status register (C_CAN_INT_REG) is set/pending. According to the manual,
> >>> reading the status register will also clear the bit in C_CAN_INT_REG.
> >>> This could explain lost (bus-off) status interrupts and also the strange
> >>> messages I mentioned above.
> >>>
> >>> Wolfgang.
> >>
> >> It sounds plausible. I can try it. Have you got a good datasheet for D_Can ?
> >
> > I have attached my (untested) patch. TXOK and RXOK is still handled
> > unconditionally, but that's maybe less critical.
>
> Has someone tested this patch?
I believe Wolfgang's analysis is right, so I tested the patch and it
doesn't work, because C_CAN_INT_REG is already read in c_can_isr.
On my BeagleBone-like board, the INT_STS_PENDING bit is gone by the
second read.
I modified the patch with an atomic_t in priv, which holds the relevant
bit.
commit 388d1d0d53fb1c000e43e1d58b91e7782d60fcfe
Author: Kurt Van Dijck <dev.kurt@xxxxxxxxxxxxxxxxxxxxxx>
Date: Thu Jul 4 22:37:11 2019
net can c_can: only read status register after status irq
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 8acdc7f..d5567a7 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -198,6 +198,7 @@ struct c_can_priv {
struct net_device *dev;
struct device *device;
atomic_t tx_active;
+ atomic_t sie_pending;
unsigned long tx_dir;
int last_status;
u16 (*read_reg) (const struct c_can_priv *priv, enum reg index);
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 606b7d8..61c7331 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -97,6 +97,9 @@
#define BTR_TSEG2_SHIFT 12
#define BTR_TSEG2_MASK (0x7 << BTR_TSEG2_SHIFT)
+/* interrupt register */
+#define INT_STS_PENDING 0x8000
+
/* brp extension register */
#define BRP_EXT_BRPE_MASK 0x0f
#define BRP_EXT_BRPE_SHIFT 0
@@ -1029,40 +1032,48 @@ static int c_can_poll(struct napi_struct *napi, int quota)
u16 curr, last = priv->last_status;
int work_done = 0;
- priv->last_status = curr = priv->read_reg(priv, C_CAN_STS_REG);
- /* Ack status on C_CAN. D_CAN is self clearing */
- if (priv->type != BOSCH_D_CAN)
- priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
+ /* Only read the status register if a status interrupt was pending */
+ if (atomic_xchg(&priv->sie_pending, 0)) {
+ priv->last_status = curr = priv->read_reg(priv, C_CAN_STS_REG);
+ /* Ack status on C_CAN. D_CAN is self clearing */
+ if (priv->type != BOSCH_D_CAN)
+ priv->write_reg(priv, C_CAN_STS_REG, LEC_UNUSED);
+
+ /* handle state changes */
+ if ((curr & STATUS_EWARN) && (!(last & STATUS_EWARN))) {
+ netdev_dbg(dev, "entered error warning state\n");
+ work_done +=
+ c_can_handle_state_change(dev,
+ C_CAN_ERROR_WARNING);
+ }
- /* handle state changes */
- if ((curr & STATUS_EWARN) && (!(last & STATUS_EWARN))) {
- netdev_dbg(dev, "entered error warning state\n");
- work_done += c_can_handle_state_change(dev, C_CAN_ERROR_WARNING);
- }
+ if ((curr & STATUS_EPASS) && (!(last & STATUS_EPASS))) {
+ netdev_dbg(dev, "entered error passive state\n");
+ work_done +=
+ c_can_handle_state_change(dev,
+ C_CAN_ERROR_PASSIVE);
+ }
- if ((curr & STATUS_EPASS) && (!(last & STATUS_EPASS))) {
- netdev_dbg(dev, "entered error passive state\n");
- work_done += c_can_handle_state_change(dev, C_CAN_ERROR_PASSIVE);
- }
+ if ((curr & STATUS_BOFF) && (!(last & STATUS_BOFF))) {
+ netdev_dbg(dev, "entered bus off state\n");
+ work_done +=
+ c_can_handle_state_change(dev, C_CAN_BUS_OFF);
+ goto end;
+ }
- if ((curr & STATUS_BOFF) && (!(last & STATUS_BOFF))) {
- netdev_dbg(dev, "entered bus off state\n");
- work_done += c_can_handle_state_change(dev, C_CAN_BUS_OFF);
- goto end;
- }
+ /* handle bus recovery events */
+ if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
+ netdev_dbg(dev, "left bus off state\n");
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ }
+ if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
+ netdev_dbg(dev, "left error passive state\n");
+ priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ }
- /* handle bus recovery events */
- if ((!(curr & STATUS_BOFF)) && (last & STATUS_BOFF)) {
- netdev_dbg(dev, "left bus off state\n");
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
+ /* handle lec errors on the bus */
+ work_done += c_can_handle_bus_err(dev, curr & LEC_MASK);
}
- if ((!(curr & STATUS_EPASS)) && (last & STATUS_EPASS)) {
- netdev_dbg(dev, "left error passive state\n");
- priv->can.state = CAN_STATE_ERROR_ACTIVE;
- }
-
- /* handle lec errors on the bus */
- work_done += c_can_handle_bus_err(dev, curr & LEC_MASK);
/* Handle Tx/Rx events. We do this unconditionally */
work_done += c_can_do_rx_poll(dev, (quota - work_done));
@@ -1083,10 +1094,16 @@ static irqreturn_t c_can_isr(int irq, void *dev_id)
{
struct net_device *dev = (struct net_device *)dev_id;
struct c_can_priv *priv = netdev_priv(dev);
+ int intreg;
- if (!priv->read_reg(priv, C_CAN_INT_REG))
+ intreg = priv->read_reg(priv, C_CAN_INT_REG);
+ if (!intreg)
return IRQ_NONE;
+ /* save for later use */
+ if (intreg & INT_STS_PENDING)
+ atomic_set(&priv->sie_pending, 1);
+
/* disable all interrupts and schedule the NAPI */
c_can_irq_control(priv, false);
napi_schedule(&priv->napi);
