can: m_can: m_can_isr IR_ERR_ALL_30X

can: m_can: m_can_isr IR_ERR_ALL_30X

[Michael Anochin]

In m_can_isr, in  rx handling the IR_ERR_ALL_30X is used without 
checking cdev->version > 30

if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_30X))
{}

May be it is better to use IR_ERR_ALL_31X as mask for m_can_ver>30?

[PATCH] can: m_can: m_can_isr: IR_ERR_ALL_30X

[Michael Anochin]

For m_can version >3.0 the correct IR_ERR_ALL_31X mask is used.
If an error occurs when processing RX status flags, the TX flags are 
processed anyway.

I am not sure, but maybe it is better to save IR to cdev->irqstatus
immediately before clear IR and not only by RX-Flags handling.

As far as I have seen, cdev->irqstatus is not accessed by tx-handling, 
therefore it does not matter.

This is my first patch, sorry if something is not right.

---
  drivers/net/can/m_can/m_can.c | 33 +++++++++++++++++++++++----------
  1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 1a4b56f6fa8c..eb9fd9c846a2 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1053,6 +1053,7 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
  {
  	struct net_device *dev = (struct net_device *)dev_id;
  	struct m_can_classdev *cdev = netdev_priv(dev);
+	int err=0;
  	u32 ir;

  	if (pm_runtime_suspended(cdev->dev))
@@ -1073,16 +1074,17 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
  	 * - state change IRQ
  	 * - bus error IRQ and bus error reporting
  	 */
-	if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_30X)) {
-		cdev->irqstatus = ir;
-		m_can_disable_all_interrupts(cdev);
-		if (!cdev->is_peripheral)
-			napi_schedule(&cdev->napi);
-		else if (m_can_rx_peripheral(dev) < 0)
-			goto out_fail;
-	}
-
  	if (cdev->version == 30) {
+
+		if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_30X)) {
+			cdev->irqstatus = ir;
+			m_can_disable_all_interrupts(cdev);
+			if (!cdev->is_peripheral)
+				napi_schedule(&cdev->napi);
+			else if (m_can_rx_peripheral(dev) < 0)
+				err=-1;
+		}
+
  		if (ir & IR_TC) {
  			/* Transmission Complete Interrupt*/
  			u32 timestamp = 0;
@@ -1095,10 +1097,18 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
  			netif_wake_queue(dev);
  		}
  	} else  {
+		if ((ir & IR_RF0N) || (ir & IR_ERR_ALL_31X)) {
+			cdev->irqstatus = ir;
+			m_can_disable_all_interrupts(cdev);
+			if (!cdev->is_peripheral)
+				napi_schedule(&cdev->napi);
+			else if (m_can_rx_peripheral(dev) < 0)
+				err=-2;
+		}
  		if (ir & IR_TEFN) {
  			/* New TX FIFO Element arrived */
  			if (m_can_echo_tx_event(dev) != 0)
-				goto out_fail;
+				err=-2;

  			can_led_event(dev, CAN_LED_EVENT_TX);
  			if (netif_queue_stopped(dev) &&
@@ -1107,6 +1117,9 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
  		}
  	}

+	if (err)
+		goto out_fail;
+
  	if (cdev->is_peripheral)
  		can_rx_offload_threaded_irq_finish(&cdev->offload);

-- 
2.20.1

Re: [PATCH] can: m_can: m_can_isr: IR_ERR_ALL_30X

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On 25.02.2022 13:20:22, Michael Anochin wrote:
> For m_can version >3.0 the correct IR_ERR_ALL_31X mask is used.

Without looking at the different register descriptions this sounds
plausible.

> If an error occurs when processing RX status flags, the TX flags are
> processed anyway.

You have basically changed the immediate "goto out_fail" to setting
"err" and doing to "goto out_fail" later. Why?

As far as I can see, the m_can_rx_peripheral() function may return an
error due to an SPI problem only. It unlikely that the TX flags handling
would work. All interrupts are disabled at the end of this function,
i.e. after goto out_fail. So the driver will not work anyways.

> I am not sure, but maybe it is better to save IR to cdev->irqstatus
> immediately before clear IR and not only by RX-Flags handling.
> 
> As far as I have seen, cdev->irqstatus is not accessed by tx-handling,
> therefore it does not matter.
> 
> This is my first patch, sorry if something is not right.

Thanks for your patch. There is a document on how to submit patches:
https://elixir.bootlin.com/linux/v5.12/source/Documentation/process/submitting-patches.rst

Please add your Signed-off-by to the next patch you send. See "Sign your
work" in that document.

Please improve the subject of the patch to describe what this patch
changes. The patch description could be improved and better explain why
the change is done.

This patch supposed to fix 2 different problems, please make one patch
per problem fix.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]