| From: Anssi Hannula <anssi.hannula@bitwise.fi> |
| Date: Tue, 7 Feb 2017 17:01:14 +0200 |
| Subject: can: xilinx_can: fix RX loop if RXNEMP is asserted without RXOK |
| |
| commit 32852c561bffd613d4ed7ec464b1e03e1b7b6c5c upstream. |
| |
| If the device gets into a state where RXNEMP (RX FIFO not empty) |
| interrupt is asserted without RXOK (new frame received successfully) |
| interrupt being asserted, xcan_rx_poll() will continue to try to clear |
| RXNEMP without actually reading frames from RX FIFO. If the RX FIFO is |
| not empty, the interrupt will not be cleared and napi_schedule() will |
| just be called again. |
| |
| This situation can occur when: |
| |
| (a) xcan_rx() returns without reading RX FIFO due to an error condition. |
| The code tries to clear both RXOK and RXNEMP but RXNEMP will not clear |
| due to a frame still being in the FIFO. The frame will never be read |
| from the FIFO as RXOK is no longer set. |
| |
| (b) A frame is received between xcan_rx_poll() reading interrupt status |
| and clearing RXOK. RXOK will be cleared, but RXNEMP will again remain |
| set as the new message is still in the FIFO. |
| |
| I'm able to trigger case (b) by flooding the bus with frames under load. |
| |
| There does not seem to be any benefit in using both RXNEMP and RXOK in |
| the way the driver does, and the polling example in the reference manual |
| (UG585 v1.10 18.3.7 Read Messages from RxFIFO) also says that either |
| RXOK or RXNEMP can be used for detecting incoming messages. |
| |
| Fix the issue and simplify the RX processing by only using RXNEMP |
| without RXOK. |
| |
| Tested with the integrated CAN on Zynq-7000 SoC. |
| |
| Fixes: b1201e44f50b ("can: xilinx CAN controller support") |
| Signed-off-by: Anssi Hannula <anssi.hannula@bitwise.fi> |
| Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> |
| Signed-off-by: Ben Hutchings <ben@decadent.org.uk> |
| --- |
| drivers/net/can/xilinx_can.c | 18 +++++------------- |
| 1 file changed, 5 insertions(+), 13 deletions(-) |
| |
| --- a/drivers/net/can/xilinx_can.c |
| +++ b/drivers/net/can/xilinx_can.c |
| @@ -100,7 +100,7 @@ enum xcan_reg { |
| #define XCAN_INTR_ALL (XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |\ |
| XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK | \ |
| XCAN_IXR_RXNEMP_MASK | XCAN_IXR_ERROR_MASK | \ |
| - XCAN_IXR_ARBLST_MASK | XCAN_IXR_RXOK_MASK) |
| + XCAN_IXR_ARBLST_MASK) |
| |
| /* CAN register bit shift - XCAN_<REG>_<BIT>_SHIFT */ |
| #define XCAN_BTR_SJW_SHIFT 7 /* Synchronous jump width */ |
| @@ -709,15 +709,7 @@ static int xcan_rx_poll(struct napi_stru |
| |
| isr = priv->read_reg(priv, XCAN_ISR_OFFSET); |
| while ((isr & XCAN_IXR_RXNEMP_MASK) && (work_done < quota)) { |
| - if (isr & XCAN_IXR_RXOK_MASK) { |
| - priv->write_reg(priv, XCAN_ICR_OFFSET, |
| - XCAN_IXR_RXOK_MASK); |
| - work_done += xcan_rx(ndev); |
| - } else { |
| - priv->write_reg(priv, XCAN_ICR_OFFSET, |
| - XCAN_IXR_RXNEMP_MASK); |
| - break; |
| - } |
| + work_done += xcan_rx(ndev); |
| priv->write_reg(priv, XCAN_ICR_OFFSET, XCAN_IXR_RXNEMP_MASK); |
| isr = priv->read_reg(priv, XCAN_ISR_OFFSET); |
| } |
| @@ -728,7 +720,7 @@ static int xcan_rx_poll(struct napi_stru |
| if (work_done < quota) { |
| napi_complete(napi); |
| ier = priv->read_reg(priv, XCAN_IER_OFFSET); |
| - ier |= (XCAN_IXR_RXOK_MASK | XCAN_IXR_RXNEMP_MASK); |
| + ier |= XCAN_IXR_RXNEMP_MASK; |
| priv->write_reg(priv, XCAN_IER_OFFSET, ier); |
| } |
| return work_done; |
| @@ -800,9 +792,9 @@ static irqreturn_t xcan_interrupt(int ir |
| } |
| |
| /* Check for the type of receive interrupt and Processing it */ |
| - if (isr & (XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK)) { |
| + if (isr & XCAN_IXR_RXNEMP_MASK) { |
| ier = priv->read_reg(priv, XCAN_IER_OFFSET); |
| - ier &= ~(XCAN_IXR_RXNEMP_MASK | XCAN_IXR_RXOK_MASK); |
| + ier &= ~XCAN_IXR_RXNEMP_MASK; |
| priv->write_reg(priv, XCAN_IER_OFFSET, ier); |
| napi_schedule(&priv->napi); |
| } |