Avoid register reading in atomic state

> Hi,
>
> I have mcp2515 chip (socketcan revision 970) with at91sam9261 (olimex
> board) and when I want to get state of the interface by typing:
>
> cat /sys/devices/platform/atmel_spi.0/spi0.3/net/can0/can_state
>
> I get this:
>
> BUG: scheduling while atomic: cat/387/0x00000002

this is my conjecture: what happens here is that some function in the
stack dump you posted calls mcp251x_do_get_state in an atomic context.
After some grepping I found that one culprit (but I'm not sure it's
alone) is can_dev_show doing a read_lock (I guess you have preemption
on otherwise the spinlock should vanish ... you don't have a SMP ARM,
do you?). The mcp251x works on a slow bus (SPI) which needs an
interrupt to finish register read so it sleeps until it's delivered
(we cannot busy loop since it would waste tons of CPU time). But in
Linux you can't sleep (or better, you cannot reschedule) while holing
a spinlock. To say the truth this is an unfortunate situation that is
quite common when dealing with devices over such slow buses (I2C, SPI,
etc.). So, we can:

1) rewrite the function above lockless. It's not that easy.

2) you can try the patch attached (but keep in mind I haven't tested
it right now, I'm away from the hardware). But beware, the reading
could become stale. What can happen is that the bus recovers but the
entry is not updated (the other way is not possible because a bus
error triggers an interrupt and the state is update). Unfortunately we
don't have a reliable way to know if we are in an atomic context (see
http://lwn.net/Articles/274695/ ) to make the guess more rightful.

I don't see other ways for solving this problem right now. Perhaps
some more skilled kernel hacker can give a better advice. I love to
hear opinions on this since I met this problem myself a couple of
times.

Signed-off-by: Daniel Lewandowski <daniel@raciazek.eu>

1 files changed, 17 insertions, 17 deletions

diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c
index 8de432f4700..1c2530b1e53 100644
--- a/drivers/net/can/mcp251x.c
+++ b/drivers/net/can/mcp251x.c
@@ -187,6 +187,7 @@ struct mcp251x_priv {
 #define AFTER_SUSPEND_DOWN 2
 #define AFTER_SUSPEND_POWER 4
 	int restart_tx;
+	uint eflag;
 };
 
 static u8 mcp251x_read_reg(struct spi_device *spi, uint8_t reg)
@@ -760,16 +761,12 @@ static int mcp251x_stop(struct net_device *net)
 static int mcp251x_do_get_state(struct net_device *net, enum can_state	*state)
 {
 	struct mcp251x_priv *priv = netdev_priv(net);
-	struct spi_device *spi = priv->spi;
-	u8 eflag;
-
-	eflag = mcp251x_read_reg(spi, EFLG);
 
-	if (eflag & EFLG_TXBO)
+	if (priv->eflag & EFLG_TXBO)
 		*state = CAN_STATE_BUS_OFF;
-	else if (eflag & (EFLG_RXEP | EFLG_TXEP))
+	else if (priv->eflag & (EFLG_RXEP | EFLG_TXEP))
 		*state = CAN_STATE_BUS_PASSIVE;
-	else if (eflag & EFLG_EWARN)
+	else if (priv->eflag & EFLG_EWARN)
 		*state = CAN_STATE_BUS_WARNING;
 	else
 		*state = CAN_STATE_ACTIVE;
@@ -786,6 +783,8 @@ static void mcp251x_tx_work_handler(struct work_struct *ws)
 
 	dev_dbg(&spi->dev, "%s\n", __func__);
 
+	priv->eflag = mcp251x_read_reg(spi, EFLG);
+
 	if (priv->tx_skb) {
 		frame = (struct can_frame *)priv->tx_skb->data;
 		if (frame->can_dlc > CAN_FRAME_MAX_DATA_LEN)
@@ -844,6 +843,8 @@ static void mcp251x_irq_work_handler(struct work_struct *ws)
 			priv->wake = 0;
 		}
 
+		priv->eflag = mcp251x_read_reg(spi, EFLG);
+
 		intf = mcp251x_read_reg(spi, CANINTF);
 		if (intf == 0x00)
 			break;
@@ -878,9 +879,8 @@ static void mcp251x_irq_work_handler(struct work_struct *ws)
 		if (intf & CANINTF_ERRIF) {
 			struct sk_buff *skb;
 			struct can_frame *frame = NULL;
-			u8 eflag = mcp251x_read_reg(spi, EFLG);
 
-			dev_dbg(&spi->dev, "EFLG = 0x%02x\n", eflag);
+			dev_dbg(&spi->dev, "EFLG = 0x%02x\n", priv->eflag);
 
 			/* Create error frame */
 			skb = dev_alloc_skb(sizeof(struct can_frame));
@@ -897,31 +897,31 @@ static void mcp251x_irq_work_handler(struct work_struct *ws)
 				skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 				/* Set error frame flags based on bus state */
-				if (eflag & EFLG_TXBO) {
+				if (priv->eflag & EFLG_TXBO) {
 					frame->can_id |= CAN_ERR_BUSOFF;
-				} else if (eflag & EFLG_TXEP) {
+				} else if (priv->eflag & EFLG_TXEP) {
 					frame->can_id |= CAN_ERR_CRTL;
 					frame->data[1] |=
 					  CAN_ERR_CRTL_TX_PASSIVE;
-				} else if (eflag & EFLG_RXEP) {
+				} else if (priv->eflag & EFLG_RXEP) {
 					frame->can_id |= CAN_ERR_CRTL;
 					frame->data[1] |=
 					  CAN_ERR_CRTL_RX_PASSIVE;
-				} else if (eflag & EFLG_TXWAR) {
+				} else if (priv->eflag & EFLG_TXWAR) {
 					frame->can_id |= CAN_ERR_CRTL;
 					frame->data[1] |=
 					  CAN_ERR_CRTL_TX_WARNING;
-				} else if (eflag & EFLG_RXWAR) {
+				} else if (priv->eflag & EFLG_RXWAR) {
 					frame->can_id |= CAN_ERR_CRTL;
 					frame->data[1] |=
 					  CAN_ERR_CRTL_RX_WARNING;
 				}
 			}
 
-			if (eflag & (EFLG_RX0OVR | EFLG_RX1OVR)) {
-				if (eflag & EFLG_RX0OVR)
+			if (priv->eflag & (EFLG_RX0OVR | EFLG_RX1OVR)) {
+				if (priv->eflag & EFLG_RX0OVR)
 					net->stats.rx_over_errors++;
-				if (eflag & EFLG_RX1OVR)
+				if (priv->eflag & EFLG_RX1OVR)
 					net->stats.rx_over_errors++;
 				if (frame) {
 					frame->can_id |= CAN_ERR_CRTL;