Re: [PATCH can-next 5/5] can: tcan4x5x: implement handling of device interrupts

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 01.06.2021 08:50:28, Torin Cooper-Bennun wrote:
> On Wed, May 26, 2021 at 05:15:59PM +0200, Marc Kleine-Budde wrote:
> > On 26.05.2021 13:47:47, Torin Cooper-Bennun wrote:
> > > Handle power, transceiver and SPI failures by printing a useful error
> > > message (multiple simultaneous failures are not logged) and disabling
> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Is this a limitation of your code or the tcan core?
> 
> My code doesn't print an error message for every handled interrupt, only
> the first, because it's very rare to see more than one. Perhaps it's
> prudent to print a line for each handled interrupt just in case.

Yes, please make it so.

> > >  static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
> > >  						  bool clear_only)
> > >  {
> > > -	tcan4x5x_clear_interrupts(cdev);
> > > +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> > > +	int err = 0;
> > > +	irqreturn_t handled = IRQ_NONE;
> > 
> > nitpick: please make "int err" the last.
> 
> ACK
> 
> > 
> > > +
> > > +	if (!clear_only) {
> > > +		u32 ir = 0;
> > > +		const char *fail_str = "";
> > 
> > nitpick: please make the u32 the last.
> 
> ACK
> 
> > > +		else if (ir & TCAN4X5X_CANDOM_INT_EN)
> > > +			fail_str = "CAN stuck dominant (CANDOM)";
> > 
> > The error message suggests, that this error can be triggered by messing
> > around with the CAN high/low wires. I'm not sure if it's a good idea to
> > shutdown the driver in this case.
> 
> ACK, but I need to test whether the device stays functional without CPU
> intervention after CANDOM is asserted.

- Does IRQ line stay asserted if the CAN lines are still stuck dominant?

If yes, this would result in an IRQ storm, which we don't want to have.
If you/we want to handle this in a "proper" way, send a CAN error frame [1],
mask this interrupt, setup a timer/workqueue/etc and unmask it after
some 100ms.
  
[1] But I'm not sure if we have proper values for stuck dominant yet.
    https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/error.h#L110
  
- What happens if the stuck dominant condition is gone?

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