[PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Torin Cooper-Bennun]

Using the TCAN4550, I've had occasions where the m_can driver has fallen
over with the "nobody cared" message -- the ISR has returned IRQ_NONE
upon "99,900 of the previous 100,000 interrupts" (see
kernel/irq/spurious.c, __report_bad_irq()).

While such high numbers certainly indicate some kind of fault,
presently, device-specific interrupts are totally ignored -- it may be
that such a fault can be handled with a device-specific action.

Comments are welcome. One thing right off the bat: I'm not sure whether
the new callback should be added alongside clear_interrupts, or if it
should replace it.

I also need to replicate the fault observed with the TCAN4550 - it's
unlikely that the handling added in this patchset is effective by
itself.

Torin Cooper-Bennun (3):
  can: m_can: add handle_dev_interrupts callback to m_can_ops
  can: m_can: m_can_isr(): handle device-specific interrupts
  can: tcan4x5x: add handle_dev_interrupts callback to ops

 drivers/net/can/m_can/m_can.c         | 13 ++++++++++++-
 drivers/net/can/m_can/m_can.h         |  2 ++
 drivers/net/can/m_can/tcan4x5x-core.c | 25 +++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 1 deletion(-)

[PATCH RFC can-next 1/3] can: m_can: add handle_dev_interrupts callback to m_can_ops

[Torin Cooper-Bennun]

This callback will allow M_CAN-based devices, e.g. TI TCAN4550, to
handle device-specific interrupts which are not part of the M_CAN core,
but are signaled on the same interrupt pin.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index ace071c3e58c..99203a37f9cc 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -28,6 +28,7 @@
 #include <linux/iopoll.h>
 #include <linux/can/dev.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/irqreturn.h>
 
 /* m_can lec values */
 enum m_can_lec_type {
@@ -62,6 +63,7 @@ struct m_can_classdev;
 struct m_can_ops {
 	/* Device specific call backs */
 	int (*clear_interrupts)(struct m_can_classdev *cdev);
+	irqreturn_t (*handle_dev_interrupts)(struct m_can_classdev *cdev);
 	u32 (*read_reg)(struct m_can_classdev *cdev, int reg);
 	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
 	u32 (*read_fifo)(struct m_can_classdev *cdev, int addr_offset);
-- 
2.30.2

Re: [PATCH RFC can-next 1/3] can: m_can: add handle_dev_interrupts callback to m_can_ops

[Marc Kleine-Budde]

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

On 14.05.2021 13:19:44, Torin Cooper-Bennun wrote:
> This callback will allow M_CAN-based devices, e.g. TI TCAN4550, to
> handle device-specific interrupts which are not part of the M_CAN core,
> but are signaled on the same interrupt pin.
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>

Another option would be to register a 2nd threaded interrupt handler in
the tcan4x5x driver....But this is much simpler.

> ---
>  drivers/net/can/m_can/m_can.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index ace071c3e58c..99203a37f9cc 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -28,6 +28,7 @@
>  #include <linux/iopoll.h>
>  #include <linux/can/dev.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/irqreturn.h>
>  
>  /* m_can lec values */
>  enum m_can_lec_type {
> @@ -62,6 +63,7 @@ struct m_can_classdev;
>  struct m_can_ops {
>  	/* Device specific call backs */
>  	int (*clear_interrupts)(struct m_can_classdev *cdev);
> +	irqreturn_t (*handle_dev_interrupts)(struct m_can_classdev *cdev);
>  	u32 (*read_reg)(struct m_can_classdev *cdev, int reg);
>  	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
>  	u32 (*read_fifo)(struct m_can_classdev *cdev, int addr_offset);
> -- 
> 2.30.2

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 --]

Re: [PATCH RFC can-next 1/3] can: m_can: add handle_dev_interrupts callback to m_can_ops

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 02:26:10PM +0200, Marc Kleine-Budde wrote:
> On 14.05.2021 13:19:44, Torin Cooper-Bennun wrote:
> > This callback will allow M_CAN-based devices, e.g. TI TCAN4550, to
> > handle device-specific interrupts which are not part of the M_CAN core,
> > but are signaled on the same interrupt pin.
> > 
> > Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> 
> Another option would be to register a 2nd threaded interrupt handler in
> the tcan4x5x driver....But this is much simpler.

Total separation of the two does make some sense. Events arising from
the device-specific interrupt handling would be things like
under-/over-voltage faults, CAN stuck dominant, SPI malfunction, etc...
things that M_CAN shouldn't have to care about at all.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 1/3] can: m_can: add handle_dev_interrupts callback to m_can_ops

[Marc Kleine-Budde]

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

On 14.05.2021 14:21:59, Torin Cooper-Bennun wrote:
> On Fri, May 14, 2021 at 02:26:10PM +0200, Marc Kleine-Budde wrote:
> > On 14.05.2021 13:19:44, Torin Cooper-Bennun wrote:
> > > This callback will allow M_CAN-based devices, e.g. TI TCAN4550, to
> > > handle device-specific interrupts which are not part of the M_CAN core,
> > > but are signaled on the same interrupt pin.
> > > 
> > > Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> > 
> > Another option would be to register a 2nd threaded interrupt handler in
> > the tcan4x5x driver....But this is much simpler.
> 
> Total separation of the two does make some sense. Events arising from
> the device-specific interrupt handling would be things like
> under-/over-voltage faults, CAN stuck dominant, SPI malfunction, etc...
> things that M_CAN shouldn't have to care about at all.

Technically you could register a 2nd interrupt handler, which is totally
independent of the main one. But there are no callbacks for open() and
stop() into the tcan4x5x driver to request and free the interrupt
handler. So having a single callback is the way to go here.

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 --]

[PATCH RFC can-next 2/3] can: m_can: m_can_isr(): handle device-specific interrupts

[Torin Cooper-Bennun]

Device-specific interrupts are handled, if applicable (and if no M_CAN
interrupts were handled in this ISR call).

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 34073cd077e4..af63dd4b8cd5 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1033,20 +1033,31 @@ 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);
 	u32 ir;
+	irqreturn_t ret = IRQ_NONE;
 
 	if (pm_runtime_suspended(cdev->dev))
 		return IRQ_NONE;
+
 	ir = m_can_read(cdev, M_CAN_IR);
 	if (!ir)
-		return IRQ_NONE;
+		ret = IRQ_NONE;
 
 	/* ACK all irqs */
 	if (ir & IR_ALL_INT)
 		m_can_write(cdev, M_CAN_IR, ir);
 
+	/* If we have no M_CAN interrupts to handle, we can assume
+	 * device-specific interrupt(s) need handling.
+	 */
+	if (!ir && cdev->ops->handle_dev_interrupts)
+		ret = cdev->ops->handle_dev_interrupts(cdev);
+
 	if (cdev->ops->clear_interrupts)
 		cdev->ops->clear_interrupts(cdev);
 
+	if (!ir)
+		return ret;
+
 	/* schedule NAPI in case of
 	 * - rx IRQ
 	 * - state change IRQ
-- 
2.30.2

[PATCH RFC can-next 3/3] can: tcan4x5x: add handle_dev_interrupts callback to ops

[Torin Cooper-Bennun]

Though the TCAN4550's device-specific interrupts are cleared in
tcan4x5x_clear_interrupts(), they are ignored, which may cause the m_can
driver to stop working due to the ISR becoming disabled (the famous
"nobody cared" message).

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 4147cecfbbd6..cee7dfff381f 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -39,6 +39,7 @@
 #define TCAN4X5X_CANBUS_ERR_INT_EN BIT(5)
 #define TCAN4X5X_BUS_FAULT BIT(4)
 #define TCAN4X5X_MCAN_INT BIT(1)
+#define TCAN4X5X_VTWD_INT BIT(0)
 #define TCAN4X5X_ENABLE_TCAN_INT \
 	(TCAN4X5X_MCAN_INT | TCAN4X5X_BUS_FAULT | \
 	 TCAN4X5X_CANBUS_ERR_INT_EN | TCAN4X5X_CANINT_INT_EN)
@@ -190,6 +191,16 @@ static int tcan4x5x_power_enable(struct regulator *reg, int enable)
 		return regulator_disable(reg);
 }
 
+static u32 tcan4x5x_read_tcan_reg(struct m_can_classdev *cdev, int reg)
+{
+	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
+	u32 val;
+
+	regmap_read(priv->regmap, reg, &val);
+
+	return val;
+}
+
 static int tcan4x5x_write_tcan_reg(struct m_can_classdev *cdev,
 				   int reg, int val)
 {
@@ -221,6 +232,19 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
 				       TCAN4X5X_CLEAR_ALL_INT);
 }
 
+static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev)
+{
+	irqreturn_t ret = IRQ_NONE;
+	int ir;
+
+	ir = tcan4x5x_read_tcan_reg(cdev, TCAN4X5X_INT_FLAGS);
+
+	if (ir & (TCAN4X5X_CANDOM_INT_EN | TCAN4X5X_VTWD_INT))
+		ret = IRQ_HANDLED;
+
+	return ret;
+}
+
 static int tcan4x5x_init(struct m_can_classdev *cdev)
 {
 	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
@@ -305,6 +329,7 @@ static struct m_can_ops tcan4x5x_ops = {
 	.write_fifo = tcan4x5x_write_fifo,
 	.read_fifo = tcan4x5x_read_fifo,
 	.clear_interrupts = tcan4x5x_clear_interrupts,
+	.handle_dev_interrupts = tcan4x5x_handle_dev_interrupts,
 };
 
 static int tcan4x5x_can_probe(struct spi_device *spi)
-- 
2.30.2

Re: [PATCH RFC can-next 3/3] can: tcan4x5x: add handle_dev_interrupts callback to ops

[Marc Kleine-Budde]

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

On 14.05.2021 13:19:46, Torin Cooper-Bennun wrote:
> Though the TCAN4550's device-specific interrupts are cleared in
> tcan4x5x_clear_interrupts(), they are ignored, which may cause the m_can
> driver to stop working due to the ISR becoming disabled (the famous
> "nobody cared" message).
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index 4147cecfbbd6..cee7dfff381f 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -39,6 +39,7 @@
>  #define TCAN4X5X_CANBUS_ERR_INT_EN BIT(5)
>  #define TCAN4X5X_BUS_FAULT BIT(4)
>  #define TCAN4X5X_MCAN_INT BIT(1)
> +#define TCAN4X5X_VTWD_INT BIT(0)
>  #define TCAN4X5X_ENABLE_TCAN_INT \
>  	(TCAN4X5X_MCAN_INT | TCAN4X5X_BUS_FAULT | \
>  	 TCAN4X5X_CANBUS_ERR_INT_EN | TCAN4X5X_CANINT_INT_EN)
> @@ -190,6 +191,16 @@ static int tcan4x5x_power_enable(struct regulator *reg, int enable)
>  		return regulator_disable(reg);
>  }
>  
> +static u32 tcan4x5x_read_tcan_reg(struct m_can_classdev *cdev, int reg)
> +{
> +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> +	u32 val;
> +
> +	regmap_read(priv->regmap, reg, &val);
> +
> +	return val;
> +}
> +
>  static int tcan4x5x_write_tcan_reg(struct m_can_classdev *cdev,
>  				   int reg, int val)
>  {
> @@ -221,6 +232,19 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
>  				       TCAN4X5X_CLEAR_ALL_INT);
>  }
>  
> +static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev)
> +{
> +	irqreturn_t ret = IRQ_NONE;
> +	int ir;
> +
> +	ir = tcan4x5x_read_tcan_reg(cdev, TCAN4X5X_INT_FLAGS);

For new code, please don't wrap the regmap_*() functions so that the
error values are ignored. I know, it's a bit annoying to always do the
"if (err) return err" dance.

As this is the interrupt handler there's not much we can do in case of
an error. In the mcp251xfd driver I print an error message and shut down
the interface. You should at least print an error message at the end of
the handle_interrupts() function.

I think it's best to have a single handle_interrupts() callback that
combines the existing clear_interrupts and this code. If you want to
save an SPI transaction and only read the TCAN4X5X_INT_FLAGS if the
M_CAN_IR is unset, pass is as 2nd parameter from the generic interrupt
handler.

> +
> +	if (ir & (TCAN4X5X_CANDOM_INT_EN | TCAN4X5X_VTWD_INT))
> +		ret = IRQ_HANDLED;
> +
> +	return ret;
> +}
> +
>  static int tcan4x5x_init(struct m_can_classdev *cdev)
>  {
>  	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
> @@ -305,6 +329,7 @@ static struct m_can_ops tcan4x5x_ops = {
>  	.write_fifo = tcan4x5x_write_fifo,
>  	.read_fifo = tcan4x5x_read_fifo,
>  	.clear_interrupts = tcan4x5x_clear_interrupts,
> +	.handle_dev_interrupts = tcan4x5x_handle_dev_interrupts,
>  };
>  
>  static int tcan4x5x_can_probe(struct spi_device *spi)

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 --]

Re: [PATCH RFC can-next 3/3] can: tcan4x5x: add handle_dev_interrupts callback to ops

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 04:10:12PM +0200, Marc Kleine-Budde wrote:
> For new code, please don't wrap the regmap_*() functions so that the
> error values are ignored. I know, it's a bit annoying to always do the
> "if (err) return err" dance.

Will do, thanks.

> As this is the interrupt handler there's not much we can do in case of
> an error. In the mcp251xfd driver I print an error message and shut down
> the interface. You should at least print an error message at the end of
> the handle_interrupts() function.

I assume you mean the handling of errors out of aforementioned
regmap_*() functions specifically? I agree we should shut down the
interface - I've previously endured serious debugging pain due to SPI
errors going unrecognised and causing havoc.

> I think it's best to have a single handle_interrupts() callback that
> combines the existing clear_interrupts and this code. If you want to
> save an SPI transaction and only read the TCAN4X5X_INT_FLAGS if the
> M_CAN_IR is unset, pass is as 2nd parameter from the generic interrupt
> handler.

Yup, agreed.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 3/3] can: tcan4x5x: add handle_dev_interrupts callback to ops

[Marc Kleine-Budde]

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

On 14.05.2021 15:51:01, Torin Cooper-Bennun wrote:
> On Fri, May 14, 2021 at 04:10:12PM +0200, Marc Kleine-Budde wrote:
> > For new code, please don't wrap the regmap_*() functions so that the
> > error values are ignored. I know, it's a bit annoying to always do the
> > "if (err) return err" dance.
> 
> Will do, thanks.
> 
> > As this is the interrupt handler there's not much we can do in case of
> > an error. In the mcp251xfd driver I print an error message and shut down
> > the interface. You should at least print an error message at the end of
> > the handle_interrupts() function.
> 
> I assume you mean the handling of errors out of aforementioned
> regmap_*() functions specifically?

ACK, I'm talking about the return value of the regmap_() functions.

> I agree we should shut down the interface - I've previously endured
> serious debugging pain due to SPI errors going unrecognised and
> causing havoc.

You can eventually do the same, return negative error value from the
tcan4x5x handler and the main IRQ handler will shut down the chip. A
return value >= 0 would be the IRQ_*.

Some background information:

You have to keep in mind, the regmap_() functions might fail due to a
non permanent problem, e.g. resource shortage (out-of-memory, etc...). I
decided it's too complicated to properly recover from those, especially
if the driver touches some FIFO pointer.

The mcp251xfd chip offers CRC check summing during read. If the driver
detect a CRC read error (that stays even after some retries), I let the
regmap_read() function to return an error that is then passed down the
call stack. And then chip will be shut down.

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 --]

Re: [PATCH RFC can-next 3/3] can: tcan4x5x: add handle_dev_interrupts callback to ops

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 05:15:48PM +0200, Marc Kleine-Budde wrote:
> On 14.05.2021 15:51:01, Torin Cooper-Bennun wrote:
> > I agree we should shut down the interface - I've previously endured
> > serious debugging pain due to SPI errors going unrecognised and
> > causing havoc.
> 
> You can eventually do the same, return negative error value from the
> tcan4x5x handler and the main IRQ handler will shut down the chip. A
> return value >= 0 would be the IRQ_*.
> 
> Some background information:
> 
> You have to keep in mind, the regmap_() functions might fail due to a
> non permanent problem, e.g. resource shortage (out-of-memory, etc...). I
> decided it's too complicated to properly recover from those, especially
> if the driver touches some FIFO pointer.
> 
> The mcp251xfd chip offers CRC check summing during read. If the driver
> detect a CRC read error (that stays even after some retries), I let the
> regmap_read() function to return an error that is then passed down the
> call stack. And then chip will be shut down.

Cool, I'll keep this in mind.

Obviously tcan4550 has no such SPI checking, but it does have ECC memory
at least. Unrecoverable ECC errors are reported in the device
interrupts, so that's a good example of something I'd want to respond to
with a shutdown!

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Marc Kleine-Budde]

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

On 14.05.2021 13:19:43, Torin Cooper-Bennun wrote:
> Using the TCAN4550, I've had occasions where the m_can driver has fallen
> over with the "nobody cared" message -- the ISR has returned IRQ_NONE
> upon "99,900 of the previous 100,000 interrupts" (see
> kernel/irq/spurious.c, __report_bad_irq()).
> 
> While such high numbers certainly indicate some kind of fault,
> presently, device-specific interrupts are totally ignored -- it may be
> that such a fault can be handled with a device-specific action.

Do you know why the tcan4x5x specific interrupts are enabled in the
first place? If no-one is handling them, just mask/disable/etc then....

> Comments are welcome. One thing right off the bat: I'm not sure whether
> the new callback should be added alongside clear_interrupts, or if it
> should replace it.

I don't see why we need two callbacks from the generic interrupt
handler, one should be enough.

> I also need to replicate the fault observed with the TCAN4550 - it's
> unlikely that the handling added in this patchset is effective by
> itself.

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 --]

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 02:34:17PM +0200, Marc Kleine-Budde wrote:
> On 14.05.2021 13:19:43, Torin Cooper-Bennun wrote:
> > Using the TCAN4550, I've had occasions where the m_can driver has fallen
> > over with the "nobody cared" message -- the ISR has returned IRQ_NONE
> > upon "99,900 of the previous 100,000 interrupts" (see
> > kernel/irq/spurious.c, __report_bad_irq()).
> > 
> > While such high numbers certainly indicate some kind of fault,
> > presently, device-specific interrupts are totally ignored -- it may be
> > that such a fault can be handled with a device-specific action.
> 
> Do you know why the tcan4x5x specific interrupts are enabled in the
> first place? If no-one is handling them, just mask/disable/etc then....

The TCAN4550 has interrupts that cannot be disabled or masked, including
those for faults involving SPI, power, and transceiver issues (e.g. CAN
stuck dominant).

> 
> > Comments are welcome. One thing right off the bat: I'm not sure whether
> > the new callback should be added alongside clear_interrupts, or if it
> > should replace it.
> 
> I don't see why we need two callbacks from the generic interrupt
> handler, one should be enough.

Fair enough, and it makes sense to always clear the device-specific
interrupts when handling them anyway.

tcan4x5x needs some cleanup re interrupt init/handling/clearing anyway,
so I'll incorporate that next time!

Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Marc Kleine-Budde]

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

On 14.05.2021 14:10:49, Torin Cooper-Bennun wrote:
> On Fri, May 14, 2021 at 02:34:17PM +0200, Marc Kleine-Budde wrote:
> > On 14.05.2021 13:19:43, Torin Cooper-Bennun wrote:
> > > Using the TCAN4550, I've had occasions where the m_can driver has fallen
> > > over with the "nobody cared" message -- the ISR has returned IRQ_NONE
> > > upon "99,900 of the previous 100,000 interrupts" (see
> > > kernel/irq/spurious.c, __report_bad_irq()).
> > > 
> > > While such high numbers certainly indicate some kind of fault,
> > > presently, device-specific interrupts are totally ignored -- it may be
> > > that such a fault can be handled with a device-specific action.
> > 
> > Do you know why the tcan4x5x specific interrupts are enabled in the
> > first place? If no-one is handling them, just mask/disable/etc then....
> 
> The TCAN4550 has interrupts that cannot be disabled or masked, including
> those for faults involving SPI, power, and transceiver issues (e.g. CAN
> stuck dominant).

Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
those clear-or-read?

> > > Comments are welcome. One thing right off the bat: I'm not sure whether
> > > the new callback should be added alongside clear_interrupts, or if it
> > > should replace it.
> > 
> > I don't see why we need two callbacks from the generic interrupt
> > handler, one should be enough.
> 
> Fair enough, and it makes sense to always clear the device-specific
> interrupts when handling them anyway.

ACK - and return irqreturn_t.

> tcan4x5x needs some cleanup re interrupt init/handling/clearing anyway,
> so I'll incorporate that next time!

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 --]

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 04:12:37PM +0200, Marc Kleine-Budde wrote:
> > The TCAN4550 has interrupts that cannot be disabled or masked, including
> > those for faults involving SPI, power, and transceiver issues (e.g. CAN
> > stuck dominant).
> 
> Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
> those clear-or-read?

In theory, for any of these, clearing the register should be sufficient
for the interrupt pin to go inactive... emphasis on "in theory".

> > > > Comments are welcome. One thing right off the bat: I'm not sure whether
> > > > the new callback should be added alongside clear_interrupts, or if it
> > > > should replace it.
> > > 
> > > I don't see why we need two callbacks from the generic interrupt
> > > handler, one should be enough.
> > 
> > Fair enough, and it makes sense to always clear the device-specific
> > interrupts when handling them anyway.
> 
> ACK - and return irqreturn_t.

Sounds good!

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Marc Kleine-Budde]

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

On 14.05.2021 15:44:34, Torin Cooper-Bennun wrote:
> On Fri, May 14, 2021 at 04:12:37PM +0200, Marc Kleine-Budde wrote:
> > > The TCAN4550 has interrupts that cannot be disabled or masked, including
> > > those for faults involving SPI, power, and transceiver issues (e.g. CAN
> > > stuck dominant).
> > 
> > Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
> > those clear-or-read?
> 
> In theory, for any of these, clearing the register should be sufficient
> for the interrupt pin to go inactive... emphasis on "in theory".

Ok, but as you noticed in your patch, if no IRQ is pending in M_CAN_IR,
the main driver doesn't call the tcan4x5x handler.

So the IRQ stays active, the IRQ handler is repeatedly called and
returns IRQ_NONE. Then you get the nobody cared warning.

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 --]

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 04:55:56PM +0200, Marc Kleine-Budde wrote:
> On 14.05.2021 15:44:34, Torin Cooper-Bennun wrote:
> > On Fri, May 14, 2021 at 04:12:37PM +0200, Marc Kleine-Budde wrote:
> > > > The TCAN4550 has interrupts that cannot be disabled or masked, including
> > > > those for faults involving SPI, power, and transceiver issues (e.g. CAN
> > > > stuck dominant).
> > > 
> > > Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
> > > those clear-or-read?
> > 
> > In theory, for any of these, clearing the register should be sufficient
> > for the interrupt pin to go inactive... emphasis on "in theory".
> 
> Ok, but as you noticed in your patch, if no IRQ is pending in M_CAN_IR,
> the main driver doesn't call the tcan4x5x handler.
> 
> So the IRQ stays active, the IRQ handler is repeatedly called and
> returns IRQ_NONE. Then you get the nobody cared warning.

I miscommunicated a bit there. I was just referring to the fact that
some of these interrupts are really broken, e.g. re-asserting for no
reason, despite what the datasheet says, yada yada.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 04:12:37PM +0200, Marc Kleine-Budde wrote:
> Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
> those clear-or-read?

An extra note on this... most of the faults worth responding to here are
serious enough that the interface should just be shut down, or at least,
the device will need to be init'd again. 

I'm surprised no handling was implemented in the first place - makes it
very difficult to debug problems when you're prototyping new kit. The
driver is written as though these interrupts are meaningless!

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Marc Kleine-Budde]

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

On 14.05.2021 15:54:30, Torin Cooper-Bennun wrote:
> On Fri, May 14, 2021 at 04:12:37PM +0200, Marc Kleine-Budde wrote:
> > Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
> > those clear-or-read?
> 
> An extra note on this... most of the faults worth responding to here are
> serious enough that the interface should just be shut down, or at least,
> the device will need to be init'd again. 
> 
> I'm surprised no handling was implemented in the first place -

At least you should write a warning message that you (as a developer)
and your users get noticed when these error hits.

> makes it very difficult to debug problems when you're prototyping new
> kit. The driver is written as though these interrupts are meaningless!

Proper error handling is not trivial....and from my experience, in most
cases the documentation of those kinds of errors and how to recover is
quite sparse, I haven't looked deep into the tcan4x5x docs though.

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 --]

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Torin Cooper-Bennun]

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

On Fri, May 14, 2021 at 05:21:31PM +0200, Marc Kleine-Budde wrote:
> On 14.05.2021 15:54:30, Torin Cooper-Bennun wrote:
> > On Fri, May 14, 2021 at 04:12:37PM +0200, Marc Kleine-Budde wrote:
> > > Ah, ok. Do they need any handling/acknowledge? You only read TCAN4X5X_INT_FLAGS, are
> > > those clear-or-read?
> > 
> > An extra note on this... most of the faults worth responding to here are
> > serious enough that the interface should just be shut down, or at least,
> > the device will need to be init'd again. 
> > 
> > I'm surprised no handling was implemented in the first place -
> 
> At least you should write a warning message that you (as a developer)
> and your users get noticed when these error hits.
> 
> > makes it very difficult to debug problems when you're prototyping new
> > kit. The driver is written as though these interrupts are meaningless!
> 
> Proper error handling is not trivial....and from my experience, in most
> cases the documentation of those kinds of errors and how to recover is
> quite sparse, I haven't looked deep into the tcan4x5x docs though.

I guess I should give credit where it's due, in that the driver works
fine without full error handling for the vast majority of cases. It may
just be that the application I'm working on is more sensitive to faults.

The docs are fairly sparse on error handling from a programmer's
perspective, yes. I've learned more from experience with this chip than
from the datasheet!

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


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

Re: [PATCH RFC can-next 0/3] m_can: support device-specific interrupt handling

[Marc Kleine-Budde]

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

On 14.05.2021 17:44:55, Torin Cooper-Bennun wrote:
> > Proper error handling is not trivial....and from my experience, in most
> > cases the documentation of those kinds of errors and how to recover is
> > quite sparse, I haven't looked deep into the tcan4x5x docs though.
> 
> I guess I should give credit where it's due, in that the driver works
> fine without full error handling for the vast majority of cases. It may
> just be that the application I'm working on is more sensitive to faults.
> 
> The docs are fairly sparse on error handling from a programmer's
> perspective, yes. I've learned more from experience with this chip than
> from the datasheet!

That's exactly what I meant :D

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 --]