[PATCH v2 1/2] can: m_can: set init flag earlier in probe

[PATCH v2 1/2] can: m_can: set init flag earlier in probe

[Matthias Schiffer]

While an m_can controller usually already has the init flag from a
hardware reset, no such reset happens on the integrated m_can_pci of the
Intel Elkhart Lake. If the CAN controller is found in an active state,
m_can_dev_setup() would fail because m_can_niso_supported() calls
m_can_cccr_update_bits(), which refuses to modify any other configuration
bits when CCCR_INIT is not set.

To avoid this issue, set CCCR_INIT before attempting to modify any other
configuration flags.

Fixes: cd5a46ce6fa6 ("can: m_can: don't enable transceiver when probing")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

v2: no changes

 drivers/net/can/m_can/m_can.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 012c3d22b01dd..47481afb9add3 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1681,6 +1681,14 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
 		return -EINVAL;
 	}
 
+	/* Forcing standby mode should be redundant, as the chip should be in
+	 * standby after a reset. Write the INIT bit anyways, should the chip
+	 * be configured by previous stage.
+	 */
+	err = m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
+	if (err)
+		return err;
+
 	if (!cdev->is_peripheral)
 		netif_napi_add(dev, &cdev->napi, m_can_poll);
 
@@ -1732,11 +1740,7 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
 		return -EINVAL;
 	}
 
-	/* Forcing standby mode should be redundant, as the chip should be in
-	 * standby after a reset. Write the INIT bit anyways, should the chip
-	 * be configured by previous stage.
-	 */
-	return m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
+	return 0;
 }
 
 static void m_can_stop(struct net_device *dev)
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Matthias Schiffer]

The interrupt line of PCI devices is interpreted as edge-triggered,
however the interrupt signal of the m_can controller integrated in Intel
Elkhart Lake CPUs appears to be generated level-triggered.

Consider the following sequence of events:

- IR register is read, interrupt X is set
- A new interrupt Y is triggered in the m_can controller
- IR register is written to acknowledge interrupt X. Y remains set in IR

As at no point in this sequence no interrupt flag is set in IR, the
m_can interrupt line will never become deasserted, and no edge will ever
be observed to trigger another run of the ISR. This was observed to
result in the TX queue of the EHL m_can to get stuck under high load,
because frames were queued to the hardware in m_can_start_xmit(), but
m_can_finish_tx() was never run to account for their successful
transmission.

To fix the issue, repeatedly read and acknowledge interrupts at the
start of the ISR until no interrupt flags are set, so the next incoming
interrupt will also result in an edge on the interrupt line.

Fixes: cab7ffc0324f ("can: m_can: add PCI glue driver for Intel Elkhart Lake")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---

v2: introduce flag is_edge_triggered, so we can avoid the loop on !m_can_pci

 drivers/net/can/m_can/m_can.c     | 21 ++++++++++++++++-----
 drivers/net/can/m_can/m_can.h     |  1 +
 drivers/net/can/m_can/m_can_pci.c |  1 +
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 47481afb9add3..2e182c3c98fed 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1207,20 +1207,31 @@ static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
 static int m_can_interrupt_handler(struct m_can_classdev *cdev)
 {
 	struct net_device *dev = cdev->net;
-	u32 ir;
+	u32 ir = 0, ir_read;
 	int ret;
 
 	if (pm_runtime_suspended(cdev->dev))
 		return IRQ_NONE;
 
-	ir = m_can_read(cdev, M_CAN_IR);
+	/* For m_can_pci, the interrupt line is interpreted as edge-triggered,
+	 * but the m_can controller generates them as level-triggered. We must
+	 * observe that IR is 0 at least once to be sure that the next
+	 * interrupt will generate an edge.
+	 */
+	while ((ir_read = m_can_read(cdev, M_CAN_IR)) != 0) {
+		ir |= ir_read;
+
+		/* ACK all irqs */
+		m_can_write(cdev, M_CAN_IR, ir);
+
+		if (!cdev->is_edge_triggered)
+			break;
+	}
+
 	m_can_coalescing_update(cdev, ir);
 	if (!ir)
 		return IRQ_NONE;
 
-	/* ACK all irqs */
-	m_can_write(cdev, M_CAN_IR, ir);
-
 	if (cdev->ops->clear_interrupts)
 		cdev->ops->clear_interrupts(cdev);
 
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 92b2bd8628e6b..8c17eb94d2f98 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -99,6 +99,7 @@ struct m_can_classdev {
 	int pm_clock_support;
 	int pm_wake_source;
 	int is_peripheral;
+	bool is_edge_triggered;
 
 	// Cached M_CAN_IE register content
 	u32 active_interrupts;
diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
index d72fe771dfc7a..f98527981402a 100644
--- a/drivers/net/can/m_can/m_can_pci.c
+++ b/drivers/net/can/m_can/m_can_pci.c
@@ -127,6 +127,7 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
 	mcan_class->pm_clock_support = 1;
 	mcan_class->pm_wake_source = 0;
 	mcan_class->can.clock.freq = id->driver_data;
+	mcan_class->is_edge_triggered = true;
 	mcan_class->ops = &m_can_pci_ops;
 
 	pci_set_drvdata(pci, mcan_class);
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Markus Schneider-Pargmann]

Hi Matthias,

On Thu, Sep 19, 2024 at 01:27:28PM GMT, Matthias Schiffer wrote:
> The interrupt line of PCI devices is interpreted as edge-triggered,
> however the interrupt signal of the m_can controller integrated in Intel

I have a similar patch though for a different setup (I didn't send it
yet). I have a tcan chip wired to a pin that is only capable of edge
interrupts.

> Elkhart Lake CPUs appears to be generated level-triggered.
> 
> Consider the following sequence of events:
> 
> - IR register is read, interrupt X is set
> - A new interrupt Y is triggered in the m_can controller
> - IR register is written to acknowledge interrupt X. Y remains set in IR
> 
> As at no point in this sequence no interrupt flag is set in IR, the
> m_can interrupt line will never become deasserted, and no edge will ever
> be observed to trigger another run of the ISR. This was observed to
> result in the TX queue of the EHL m_can to get stuck under high load,
> because frames were queued to the hardware in m_can_start_xmit(), but
> m_can_finish_tx() was never run to account for their successful
> transmission.
> 
> To fix the issue, repeatedly read and acknowledge interrupts at the
> start of the ISR until no interrupt flags are set, so the next incoming
> interrupt will also result in an edge on the interrupt line.
> 
> Fixes: cab7ffc0324f ("can: m_can: add PCI glue driver for Intel Elkhart Lake")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> v2: introduce flag is_edge_triggered, so we can avoid the loop on !m_can_pci
> 
>  drivers/net/can/m_can/m_can.c     | 21 ++++++++++++++++-----
>  drivers/net/can/m_can/m_can.h     |  1 +
>  drivers/net/can/m_can/m_can_pci.c |  1 +
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 47481afb9add3..2e182c3c98fed 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1207,20 +1207,31 @@ static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
>  static int m_can_interrupt_handler(struct m_can_classdev *cdev)
>  {
>  	struct net_device *dev = cdev->net;
> -	u32 ir;
> +	u32 ir = 0, ir_read;
>  	int ret;
>  
>  	if (pm_runtime_suspended(cdev->dev))
>  		return IRQ_NONE;
>  
> -	ir = m_can_read(cdev, M_CAN_IR);
> +	/* For m_can_pci, the interrupt line is interpreted as edge-triggered,
> +	 * but the m_can controller generates them as level-triggered. We must
> +	 * observe that IR is 0 at least once to be sure that the next
> +	 * interrupt will generate an edge.
> +	 */

Could you please remove this hardware specific comment? As mentioned
above this will be independent of any specific hardware.

> +	while ((ir_read = m_can_read(cdev, M_CAN_IR)) != 0) {
> +		ir |= ir_read;
> +
> +		/* ACK all irqs */
> +		m_can_write(cdev, M_CAN_IR, ir);
> +
> +		if (!cdev->is_edge_triggered)
> +			break;
> +	}
> +
>  	m_can_coalescing_update(cdev, ir);
>  	if (!ir)
>  		return IRQ_NONE;
>  
> -	/* ACK all irqs */
> -	m_can_write(cdev, M_CAN_IR, ir);
> -
>  	if (cdev->ops->clear_interrupts)
>  		cdev->ops->clear_interrupts(cdev);
>  
> diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> index 92b2bd8628e6b..8c17eb94d2f98 100644
> --- a/drivers/net/can/m_can/m_can.h
> +++ b/drivers/net/can/m_can/m_can.h
> @@ -99,6 +99,7 @@ struct m_can_classdev {
>  	int pm_clock_support;
>  	int pm_wake_source;
>  	int is_peripheral;
> +	bool is_edge_triggered;

To avoid confusion could you rename it to irq_edge_triggered or
something similar, to make clear that it is not about the chip but the
way the interrupt line is connected?

Also I am not sure it is possible, but could you use
irq_get_trigger_type() to see if it is a level or edge based interrupt?
Then we wouldn't need this additional parameter at all and could just
detect it in m_can.c.

Best
Markus

>  
>  	// Cached M_CAN_IE register content
>  	u32 active_interrupts;
> diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> index d72fe771dfc7a..f98527981402a 100644
> --- a/drivers/net/can/m_can/m_can_pci.c
> +++ b/drivers/net/can/m_can/m_can_pci.c
> @@ -127,6 +127,7 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
>  	mcan_class->pm_clock_support = 1;
>  	mcan_class->pm_wake_source = 0;
>  	mcan_class->can.clock.freq = id->driver_data;
> +	mcan_class->is_edge_triggered = true;
>  	mcan_class->ops = &m_can_pci_ops;
>  
>  	pci_set_drvdata(pci, mcan_class);
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
> 

Re: [PATCH v2 1/2] can: m_can: set init flag earlier in probe

[Markus Schneider-Pargmann]

On Thu, Sep 19, 2024 at 01:27:27PM GMT, Matthias Schiffer wrote:
> While an m_can controller usually already has the init flag from a
> hardware reset, no such reset happens on the integrated m_can_pci of the
> Intel Elkhart Lake. If the CAN controller is found in an active state,
> m_can_dev_setup() would fail because m_can_niso_supported() calls
> m_can_cccr_update_bits(), which refuses to modify any other configuration
> bits when CCCR_INIT is not set.
> 
> To avoid this issue, set CCCR_INIT before attempting to modify any other
> configuration flags.
> 
> Fixes: cd5a46ce6fa6 ("can: m_can: don't enable transceiver when probing")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
> 
> v2: no changes
> 
>  drivers/net/can/m_can/m_can.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 012c3d22b01dd..47481afb9add3 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1681,6 +1681,14 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
>  		return -EINVAL;
>  	}
>  
> +	/* Forcing standby mode should be redundant, as the chip should be in
> +	 * standby after a reset. Write the INIT bit anyways, should the chip
> +	 * be configured by previous stage.
> +	 */

Could you please update the comment to reflect your findings?

Best
Markus

> +	err = m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
> +	if (err)
> +		return err;
> +
>  	if (!cdev->is_peripheral)
>  		netif_napi_add(dev, &cdev->napi, m_can_poll);
>  
> @@ -1732,11 +1740,7 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Forcing standby mode should be redundant, as the chip should be in
> -	 * standby after a reset. Write the INIT bit anyways, should the chip
> -	 * be configured by previous stage.
> -	 */
> -	return m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT);
> +	return 0;
>  }
>  
>  static void m_can_stop(struct net_device *dev)
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/
> 

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Matthias Schiffer]

On Mon, 2024-09-23 at 10:03 +0200, Markus Schneider-Pargmann wrote:
> Hi Matthias,
> 
> On Thu, Sep 19, 2024 at 01:27:28PM GMT, Matthias Schiffer wrote:
> > The interrupt line of PCI devices is interpreted as edge-triggered,
> > however the interrupt signal of the m_can controller integrated in Intel
> 
> I have a similar patch though for a different setup (I didn't send it
> yet). I have a tcan chip wired to a pin that is only capable of edge
> interrupts.

Should I also change the Fixes tag to something else then?

> 
> > Elkhart Lake CPUs appears to be generated level-triggered.
> > 
> > Consider the following sequence of events:
> > 
> > - IR register is read, interrupt X is set
> > - A new interrupt Y is triggered in the m_can controller
> > - IR register is written to acknowledge interrupt X. Y remains set in IR
> > 
> > As at no point in this sequence no interrupt flag is set in IR, the
> > m_can interrupt line will never become deasserted, and no edge will ever
> > be observed to trigger another run of the ISR. This was observed to
> > result in the TX queue of the EHL m_can to get stuck under high load,
> > because frames were queued to the hardware in m_can_start_xmit(), but
> > m_can_finish_tx() was never run to account for their successful
> > transmission.
> > 
> > To fix the issue, repeatedly read and acknowledge interrupts at the
> > start of the ISR until no interrupt flags are set, so the next incoming
> > interrupt will also result in an edge on the interrupt line.
> > 
> > Fixes: cab7ffc0324f ("can: m_can: add PCI glue driver for Intel Elkhart Lake")
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > ---
> > 
> > v2: introduce flag is_edge_triggered, so we can avoid the loop on !m_can_pci
> > 
> >  drivers/net/can/m_can/m_can.c     | 21 ++++++++++++++++-----
> >  drivers/net/can/m_can/m_can.h     |  1 +
> >  drivers/net/can/m_can/m_can_pci.c |  1 +
> >  3 files changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index 47481afb9add3..2e182c3c98fed 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1207,20 +1207,31 @@ static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
> >  static int m_can_interrupt_handler(struct m_can_classdev *cdev)
> >  {
> >  	struct net_device *dev = cdev->net;
> > -	u32 ir;
> > +	u32 ir = 0, ir_read;
> >  	int ret;
> >  
> >  	if (pm_runtime_suspended(cdev->dev))
> >  		return IRQ_NONE;
> >  
> > -	ir = m_can_read(cdev, M_CAN_IR);
> > +	/* For m_can_pci, the interrupt line is interpreted as edge-triggered,
> > +	 * but the m_can controller generates them as level-triggered. We must
> > +	 * observe that IR is 0 at least once to be sure that the next
> > +	 * interrupt will generate an edge.
> > +	 */
> 
> Could you please remove this hardware specific comment? As mentioned
> above this will be independent of any specific hardware.

Ok.


> 
> > +	while ((ir_read = m_can_read(cdev, M_CAN_IR)) != 0) {
> > +		ir |= ir_read;
> > +
> > +		/* ACK all irqs */
> > +		m_can_write(cdev, M_CAN_IR, ir);
> > +
> > +		if (!cdev->is_edge_triggered)
> > +			break;
> > +	}
> > +
> >  	m_can_coalescing_update(cdev, ir);
> >  	if (!ir)
> >  		return IRQ_NONE;
> >  
> > -	/* ACK all irqs */
> > -	m_can_write(cdev, M_CAN_IR, ir);
> > -
> >  	if (cdev->ops->clear_interrupts)
> >  		cdev->ops->clear_interrupts(cdev);
> >  
> > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > index 92b2bd8628e6b..8c17eb94d2f98 100644
> > --- a/drivers/net/can/m_can/m_can.h
> > +++ b/drivers/net/can/m_can/m_can.h
> > @@ -99,6 +99,7 @@ struct m_can_classdev {
> >  	int pm_clock_support;
> >  	int pm_wake_source;
> >  	int is_peripheral;
> > +	bool is_edge_triggered;
> 
> To avoid confusion could you rename it to irq_edge_triggered or
> something similar, to make clear that it is not about the chip but the
> way the interrupt line is connected?

Will do.

> 
> Also I am not sure it is possible, but could you use
> irq_get_trigger_type() to see if it is a level or edge based interrupt?
> Then we wouldn't need this additional parameter at all and could just
> detect it in m_can.c.

Unfortunately that doesn't seem to work. irq_get_trigger_type() only returns a meaningful value
after the IRQ has been requested. I thought about requesting the IRQ with IRQF_NO_AUTOEN and then
filling in the irq_edge_triggered field before enabling the IRQ, but IRQF_NO_AUTOEN is incompatible
with IRQF_SHARED.

Of course there are ways around this - checking irq_get_trigger_type() from the ISR itself, or
adding more locking, but neither seems quite worthwhile to me. Would you agree with this?

Maybe there is some other way to find out the trigger type that would be set when the IRQ is
requested? I don't know what that would be however - so I'd just keep setting the flag statically
for m_can_pci and leave a dynamic solution for future improvement.

Matthias



> 
> Best
> Markus
> 
> >  
> >  	// Cached M_CAN_IE register content
> >  	u32 active_interrupts;
> > diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> > index d72fe771dfc7a..f98527981402a 100644
> > --- a/drivers/net/can/m_can/m_can_pci.c
> > +++ b/drivers/net/can/m_can/m_can_pci.c
> > @@ -127,6 +127,7 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
> >  	mcan_class->pm_clock_support = 1;
> >  	mcan_class->pm_wake_source = 0;
> >  	mcan_class->can.clock.freq = id->driver_data;
> > +	mcan_class->is_edge_triggered = true;
> >  	mcan_class->ops = &m_can_pci_ops;
> >  
> >  	pci_set_drvdata(pci, mcan_class);
> > -- 
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > https://www.tq-group.com/
> > 

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Markus Schneider-Pargmann]

On Mon, Sep 23, 2024 at 11:32:49AM GMT, Matthias Schiffer wrote:
> On Mon, 2024-09-23 at 10:03 +0200, Markus Schneider-Pargmann wrote:
> > Hi Matthias,
> > 
> > On Thu, Sep 19, 2024 at 01:27:28PM GMT, Matthias Schiffer wrote:
> > > The interrupt line of PCI devices is interpreted as edge-triggered,
> > > however the interrupt signal of the m_can controller integrated in Intel
> > 
> > I have a similar patch though for a different setup (I didn't send it
> > yet). I have a tcan chip wired to a pin that is only capable of edge
> > interrupts.
> 
> Should I also change the Fixes tag to something else then?

No, my use-case wasn't explicitly supported before, so I don't consider
it to be a fix but a new feature supporting edge triggered tcan.

> 
> > 
> > > Elkhart Lake CPUs appears to be generated level-triggered.
> > > 
> > > Consider the following sequence of events:
> > > 
> > > - IR register is read, interrupt X is set
> > > - A new interrupt Y is triggered in the m_can controller
> > > - IR register is written to acknowledge interrupt X. Y remains set in IR
> > > 
> > > As at no point in this sequence no interrupt flag is set in IR, the
> > > m_can interrupt line will never become deasserted, and no edge will ever
> > > be observed to trigger another run of the ISR. This was observed to
> > > result in the TX queue of the EHL m_can to get stuck under high load,
> > > because frames were queued to the hardware in m_can_start_xmit(), but
> > > m_can_finish_tx() was never run to account for their successful
> > > transmission.
> > > 
> > > To fix the issue, repeatedly read and acknowledge interrupts at the
> > > start of the ISR until no interrupt flags are set, so the next incoming
> > > interrupt will also result in an edge on the interrupt line.
> > > 
> > > Fixes: cab7ffc0324f ("can: m_can: add PCI glue driver for Intel Elkhart Lake")
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > > ---
> > > 
> > > v2: introduce flag is_edge_triggered, so we can avoid the loop on !m_can_pci
> > > 
> > >  drivers/net/can/m_can/m_can.c     | 21 ++++++++++++++++-----
> > >  drivers/net/can/m_can/m_can.h     |  1 +
> > >  drivers/net/can/m_can/m_can_pci.c |  1 +
> > >  3 files changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > > index 47481afb9add3..2e182c3c98fed 100644
> > > --- a/drivers/net/can/m_can/m_can.c
> > > +++ b/drivers/net/can/m_can/m_can.c
> > > @@ -1207,20 +1207,31 @@ static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
> > >  static int m_can_interrupt_handler(struct m_can_classdev *cdev)
> > >  {
> > >  	struct net_device *dev = cdev->net;
> > > -	u32 ir;
> > > +	u32 ir = 0, ir_read;
> > >  	int ret;
> > >  
> > >  	if (pm_runtime_suspended(cdev->dev))
> > >  		return IRQ_NONE;
> > >  
> > > -	ir = m_can_read(cdev, M_CAN_IR);
> > > +	/* For m_can_pci, the interrupt line is interpreted as edge-triggered,
> > > +	 * but the m_can controller generates them as level-triggered. We must
> > > +	 * observe that IR is 0 at least once to be sure that the next
> > > +	 * interrupt will generate an edge.
> > > +	 */
> > 
> > Could you please remove this hardware specific comment? As mentioned
> > above this will be independent of any specific hardware.
> 
> Ok.
> 
> 
> > 
> > > +	while ((ir_read = m_can_read(cdev, M_CAN_IR)) != 0) {
> > > +		ir |= ir_read;
> > > +
> > > +		/* ACK all irqs */
> > > +		m_can_write(cdev, M_CAN_IR, ir);
> > > +
> > > +		if (!cdev->is_edge_triggered)
> > > +			break;
> > > +	}
> > > +
> > >  	m_can_coalescing_update(cdev, ir);
> > >  	if (!ir)
> > >  		return IRQ_NONE;
> > >  
> > > -	/* ACK all irqs */
> > > -	m_can_write(cdev, M_CAN_IR, ir);
> > > -
> > >  	if (cdev->ops->clear_interrupts)
> > >  		cdev->ops->clear_interrupts(cdev);
> > >  
> > > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > > index 92b2bd8628e6b..8c17eb94d2f98 100644
> > > --- a/drivers/net/can/m_can/m_can.h
> > > +++ b/drivers/net/can/m_can/m_can.h
> > > @@ -99,6 +99,7 @@ struct m_can_classdev {
> > >  	int pm_clock_support;
> > >  	int pm_wake_source;
> > >  	int is_peripheral;
> > > +	bool is_edge_triggered;
> > 
> > To avoid confusion could you rename it to irq_edge_triggered or
> > something similar, to make clear that it is not about the chip but the
> > way the interrupt line is connected?
> 
> Will do.
> 
> > 
> > Also I am not sure it is possible, but could you use
> > irq_get_trigger_type() to see if it is a level or edge based interrupt?
> > Then we wouldn't need this additional parameter at all and could just
> > detect it in m_can.c.
> 
> Unfortunately that doesn't seem to work. irq_get_trigger_type() only returns a meaningful value
> after the IRQ has been requested. I thought about requesting the IRQ with IRQF_NO_AUTOEN and then
> filling in the irq_edge_triggered field before enabling the IRQ, but IRQF_NO_AUTOEN is incompatible
> with IRQF_SHARED.

The mentioned function works for me on ARM and DT even before
irq_request_threaded_irq() was called.

Also I am probably missing something here. Afer requesting the irq, the
interrupts are not enabled yet right? So can't you just request it and
check the triggertype immediately afterwards?

> 
> Of course there are ways around this - checking irq_get_trigger_type() from the ISR itself, or
> adding more locking, but neither seems quite worthwhile to me. Would you agree with this?
> 
> Maybe there is some other way to find out the trigger type that would be set when the IRQ is
> requested? I don't know what that would be however - so I'd just keep setting the flag statically
> for m_can_pci and leave a dynamic solution for future improvement.

No if it doesn't work easily the parameter is probably the best option.

Best
Markus

> 
> Matthias
> 
> 
> 
> > 
> > Best
> > Markus
> > 
> > >  
> > >  	// Cached M_CAN_IE register content
> > >  	u32 active_interrupts;
> > > diff --git a/drivers/net/can/m_can/m_can_pci.c b/drivers/net/can/m_can/m_can_pci.c
> > > index d72fe771dfc7a..f98527981402a 100644
> > > --- a/drivers/net/can/m_can/m_can_pci.c
> > > +++ b/drivers/net/can/m_can/m_can_pci.c
> > > @@ -127,6 +127,7 @@ static int m_can_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
> > >  	mcan_class->pm_clock_support = 1;
> > >  	mcan_class->pm_wake_source = 0;
> > >  	mcan_class->can.clock.freq = id->driver_data;
> > > +	mcan_class->is_edge_triggered = true;
> > >  	mcan_class->ops = &m_can_pci_ops;
> > >  
> > >  	pci_set_drvdata(pci, mcan_class);
> > > -- 
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > > https://www.tq-group.com/
> > > 
> 
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> https://www.tq-group.com/

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Marc Kleine-Budde]

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

On 19.09.2024 13:27:28, Matthias Schiffer wrote:
> The interrupt line of PCI devices is interpreted as edge-triggered,
> however the interrupt signal of the m_can controller integrated in Intel
> Elkhart Lake CPUs appears to be generated level-triggered.
> 
> Consider the following sequence of events:
> 
> - IR register is read, interrupt X is set
> - A new interrupt Y is triggered in the m_can controller
> - IR register is written to acknowledge interrupt X. Y remains set in IR
> 
> As at no point in this sequence no interrupt flag is set in IR, the
> m_can interrupt line will never become deasserted, and no edge will ever
> be observed to trigger another run of the ISR. This was observed to
> result in the TX queue of the EHL m_can to get stuck under high load,
> because frames were queued to the hardware in m_can_start_xmit(), but
> m_can_finish_tx() was never run to account for their successful
> transmission.
> 
> To fix the issue, repeatedly read and acknowledge interrupts at the
> start of the ISR until no interrupt flags are set, so the next incoming
> interrupt will also result in an edge on the interrupt line.
> 
> Fixes: cab7ffc0324f ("can: m_can: add PCI glue driver for Intel Elkhart Lake")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>

My coworker Lucas pointed me to:

| https://wiki.linuxfoundation.org/networking/napi#non-level_sensitive_irqs

On the other hand, I would also like to convert the !peripteral part of
the driver to rx-offload. However, I am still looking for potential
customers for this task. I have talked to some TI and ST people at LPC,
maybe they are interested.

I think let's first fix edge sensitive IRQs, then rework the driver to
rx-offload.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Marc Kleine-Budde]

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

On 23.09.2024 10:03:24, Markus Schneider-Pargmann wrote:
> Hi Matthias,
> 
> On Thu, Sep 19, 2024 at 01:27:28PM GMT, Matthias Schiffer wrote:
> > The interrupt line of PCI devices is interpreted as edge-triggered,
> > however the interrupt signal of the m_can controller integrated in Intel
> 
> I have a similar patch though for a different setup (I didn't send it
> yet). I have a tcan chip wired to a pin that is only capable of edge
> interrupts.

Can you post your patch for completeness?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Marc Kleine-Budde]

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

On 23.09.2024 12:07:33, Markus Schneider-Pargmann wrote:
> > > > diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
> > > > index 92b2bd8628e6b..8c17eb94d2f98 100644
> > > > --- a/drivers/net/can/m_can/m_can.h
> > > > +++ b/drivers/net/can/m_can/m_can.h
> > > > @@ -99,6 +99,7 @@ struct m_can_classdev {
> > > >  	int pm_clock_support;
> > > >  	int pm_wake_source;
> > > >  	int is_peripheral;
> > > > +	bool is_edge_triggered;
> > > 
> > > To avoid confusion could you rename it to irq_edge_triggered or
> > > something similar, to make clear that it is not about the chip but the
> > > way the interrupt line is connected?
> > 
> > Will do.
> > 
> > > 
> > > Also I am not sure it is possible, but could you use
> > > irq_get_trigger_type() to see if it is a level or edge based interrupt?
> > > Then we wouldn't need this additional parameter at all and could just
> > > detect it in m_can.c.
> > 
> > Unfortunately that doesn't seem to work. irq_get_trigger_type() only returns a meaningful value
> > after the IRQ has been requested. I thought about requesting the IRQ with IRQF_NO_AUTOEN and then
> > filling in the irq_edge_triggered field before enabling the IRQ, but IRQF_NO_AUTOEN is incompatible
> > with IRQF_SHARED.
> 
> The mentioned function works for me on ARM and DT even before
> irq_request_threaded_irq() was called.
> 
> Also I am probably missing something here. Afer requesting the irq, the
> interrupts are not enabled yet right? So can't you just request it and
> check the triggertype immediately afterwards?

You have to distinguish between the IRQ controller and the IRQ source in
your device. You must be able to handle IRQs right after request_irq().
If you IP core is in reset or has all IRQs masked, this will probably
not happen, but with shared IRQ one of the other IRQ sources on the IRQ
can trigger and your IRQ handler will be called. See
CONFIG_DEBUG_SHIRQ_FIXME and CONFIG_DEBUG_SHIRQ_FIXME.

> > Of course there are ways around this - checking irq_get_trigger_type() from the ISR itself, or
> > adding more locking, but neither seems quite worthwhile to me. Would you agree with this?
> > 
> > Maybe there is some other way to find out the trigger type that would be set when the IRQ is
> > requested? I don't know what that would be however - so I'd just keep setting the flag statically
> > for m_can_pci and leave a dynamic solution for future improvement.
> 
> No if it doesn't work easily the parameter is probably the best option.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH v2 2/2] can: m_can: fix missed interrupts with m_can_pci

[Matthias Schiffer]

On Mon, 2024-09-23 at 12:17 +0200, Marc Kleine-Budde wrote:
> On 19.09.2024 13:27:28, Matthias Schiffer wrote:
> > The interrupt line of PCI devices is interpreted as edge-triggered,
> > however the interrupt signal of the m_can controller integrated in Intel
> > Elkhart Lake CPUs appears to be generated level-triggered.
> > 
> > Consider the following sequence of events:
> > 
> > - IR register is read, interrupt X is set
> > - A new interrupt Y is triggered in the m_can controller
> > - IR register is written to acknowledge interrupt X. Y remains set in IR
> > 
> > As at no point in this sequence no interrupt flag is set in IR, the
> > m_can interrupt line will never become deasserted, and no edge will ever
> > be observed to trigger another run of the ISR. This was observed to
> > result in the TX queue of the EHL m_can to get stuck under high load,
> > because frames were queued to the hardware in m_can_start_xmit(), but
> > m_can_finish_tx() was never run to account for their successful
> > transmission.
> > 
> > To fix the issue, repeatedly read and acknowledge interrupts at the
> > start of the ISR until no interrupt flags are set, so the next incoming
> > interrupt will also result in an edge on the interrupt line.
> > 
> > Fixes: cab7ffc0324f ("can: m_can: add PCI glue driver for Intel Elkhart Lake")
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> 
> My coworker Lucas pointed me to:
> 
> > https://wiki.linuxfoundation.org/networking/napi#non-level_sensitive_irqs
> 

Thanks. I don't think this is directly applicable here - in our case the lost TX complete interrupts
were the issue, not (only) the RX interrupts.

Matthias


> On the other hand, I would also like to convert the !peripteral part of
> the driver to rx-offload. However, I am still looking for potential
> customers for this task. I have talked to some TI and ST people at LPC,
> maybe they are interested.
> 
> I think let's first fix edge sensitive IRQs, then rework the driver to
> rx-offload.
> 
> regards,
> Marc
> 
>  

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH DO_NOT_APPLY] net: can: m_can: Support tcan level with edge interrupts

[Markus Schneider-Pargmann]

The tcan chip has a low level interrupt line that needs to be used.
There are some SoCs and components that do only support edge interrupts
on GPIOs. In the exact example someone wired the tcan chip to a am62
GPIO.

This patch creates a workaround for these situations, enabling the use
of tcan with a falling edge interrupt. Note that this is not the
preferred way to wire a tcan chip to the SoC.

I am detecting the situation by reading the IRQ type. If it is a level
interrupt everything stays the same. Otherwise these were my
considerations and solutions:

With falling edge interrupts we have following issues:
- While handling a IRQF_ONESHOT interrupt the interrupt may be masked as
  long as the interrupt is handled. So if a new interrupt hits during
  the handling of the interrupt may be lost as it is masked. With level
  interrupts that is not a problem because the interrupt line is still
  active/low after the handler is unmasked so it will jump back into
  handling interrupts afterwards. With edge interrupts we will just
  loose the interrupt at this point as we do not see the edge while the
  interrupt is masked. Solution here is to remove the IRQF_ONESHOT flag
  in case edge interrupts are used.
- Reading and clearing the interrupt register is not atomic. So the
  interrupts we clear from the interrupt register may not result in a
  completely cleared interrupt register and leave some unhandled
  interrupt. Again this is fine for level based interrupts as they will
  be causing a new call of the interrupt handler. With edge interrupts
  we will be missing this interrupt. So we need to make sure that the
  clearing of the interrupt register actually cleared it and the
  interrupt line could have gone back to inactive/high. To do that the
  interrupt register is read/cleared/handled repeatedly until it is 0.

Updating the interrupts for coalescing is only done once at the end with
all interrupts that were handled and not for every loop. We don't want
to change interrupts multiple times here.

Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
---

This is the draft that I had for edge interrupts. For am62 I will create
a followup patch that covers minor things like IRQF_ONESHOT removal etc.

Best
Markus

 drivers/net/can/m_can/m_can.c | 114 +++++++++++++++++++++-------------
 drivers/net/can/m_can/m_can.h |   1 +
 2 files changed, 73 insertions(+), 42 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 663eb4247029..4b969f29ba55 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1208,6 +1208,7 @@ static void m_can_coalescing_update(struct m_can_classdev *cdev, u32 ir)
 static int m_can_interrupt_handler(struct m_can_classdev *cdev)
 {
 	struct net_device *dev = cdev->net;
+	u32 all_interrupts = 0;
 	u32 ir;
 	int ret;
 
@@ -1215,56 +1216,75 @@ static int m_can_interrupt_handler(struct m_can_classdev *cdev)
 		return IRQ_NONE;
 
 	ir = m_can_read(cdev, M_CAN_IR);
-	m_can_coalescing_update(cdev, ir);
-	if (!ir)
+	all_interrupts |= ir;
+	if (!ir) {
+		m_can_coalescing_update(cdev, 0);
 		return IRQ_NONE;
-
-	/* ACK all irqs */
-	m_can_write(cdev, M_CAN_IR, ir);
-
-	if (cdev->ops->clear_interrupts)
-		cdev->ops->clear_interrupts(cdev);
-
-	/* schedule NAPI in case of
-	 * - rx IRQ
-	 * - state change IRQ
-	 * - bus error IRQ and bus error reporting
-	 */
-	if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) {
-		cdev->irqstatus = ir;
-		if (!cdev->is_peripheral) {
-			m_can_disable_all_interrupts(cdev);
-			napi_schedule(&cdev->napi);
-		} else {
-			ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
-			if (ret < 0)
-				return ret;
-		}
 	}
 
-	if (cdev->version == 30) {
-		if (ir & IR_TC) {
-			/* Transmission Complete Interrupt*/
-			u32 timestamp = 0;
-			unsigned int frame_len;
+	do {
+		/* ACK all irqs */
+		m_can_write(cdev, M_CAN_IR, ir);
 
-			if (cdev->is_peripheral)
-				timestamp = m_can_get_timestamp(cdev);
-			frame_len = m_can_tx_update_stats(cdev, 0, timestamp);
-			m_can_finish_tx(cdev, 1, frame_len);
+		if (cdev->ops->clear_interrupts)
+			cdev->ops->clear_interrupts(cdev);
+
+		/* schedule NAPI in case of
+		 * - rx IRQ
+		 * - state change IRQ
+		 * - bus error IRQ and bus error reporting
+		 */
+		if (ir & (IR_RF0N | IR_RF0W | IR_ERR_ALL_30X)) {
+			cdev->irqstatus = ir;
+			if (!cdev->is_peripheral) {
+				m_can_disable_all_interrupts(cdev);
+				napi_schedule(&cdev->napi);
+			} else {
+				ret = m_can_rx_handler(dev, NAPI_POLL_WEIGHT, ir);
+				if (ret < 0)
+					return ret;
+			}
 		}
-	} else  {
-		if (ir & (IR_TEFN | IR_TEFW)) {
-			/* New TX FIFO Element arrived */
-			ret = m_can_echo_tx_event(dev);
-			if (ret != 0)
-				return ret;
+
+		if (cdev->version == 30) {
+			if (ir & IR_TC) {
+				/* Transmission Complete Interrupt*/
+				u32 timestamp = 0;
+				unsigned int frame_len;
+
+				if (cdev->is_peripheral)
+					timestamp = m_can_get_timestamp(cdev);
+				frame_len = m_can_tx_update_stats(cdev, 0, timestamp);
+				m_can_finish_tx(cdev, 1, frame_len);
+			}
+		} else  {
+			if (ir & (IR_TEFN | IR_TEFW)) {
+				/* New TX FIFO Element arrived */
+				ret = m_can_echo_tx_event(dev);
+				if (ret != 0)
+					return ret;
+			}
 		}
-	}
+		if (!cdev->irq_type_edge)
+			break;
+
+
+		/* For edge interrupts we need to read the IR register again to
+		 * check that everything is cleared. If it is not, we can not
+		 * make sure the interrupt line is inactive again which is
+		 * required at this point to not miss any new interrupts. So in
+		 * case there are interrupts signaled in IR we repeat the
+		 * interrupt handling.
+		 */
+		ir = m_can_read(cdev, M_CAN_IR);
+		all_interrupts |= ir;
+	} while (ir);
 
 	if (cdev->is_peripheral)
 		can_rx_offload_threaded_irq_finish(&cdev->offload);
 
+	m_can_coalescing_update(cdev, all_interrupts);
+
 	return IRQ_HANDLED;
 }
 
@@ -2009,6 +2029,11 @@ static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
 	return HRTIMER_RESTART;
 }
 
+static irqreturn_t m_can_hardirq(int irq, void *dev_id)
+{
+	return IRQ_WAKE_THREAD;
+}
+
 static int m_can_open(struct net_device *dev)
 {
 	struct m_can_classdev *cdev = netdev_priv(dev);
@@ -2034,6 +2059,9 @@ static int m_can_open(struct net_device *dev)
 
 	/* register interrupt handler */
 	if (cdev->is_peripheral) {
+		cdev->irq_type_edge = !(irq_get_trigger_type(dev->irq) &
+					IRQ_TYPE_LEVEL_MASK);
+
 		cdev->tx_wq = alloc_ordered_workqueue("mcan_wq",
 						      WQ_FREEZABLE | WQ_MEM_RECLAIM);
 		if (!cdev->tx_wq) {
@@ -2046,9 +2074,11 @@ static int m_can_open(struct net_device *dev)
 			INIT_WORK(&cdev->tx_ops[i].work, m_can_tx_work_queue);
 		}
 
-		err = request_threaded_irq(dev->irq, NULL, m_can_isr,
-					   IRQF_ONESHOT,
+		err = request_threaded_irq(dev->irq, m_can_hardirq, m_can_isr,
+					   (cdev->irq_type_edge ? 0 : IRQF_ONESHOT),
 					   dev->name, dev);
+		if (cdev->irq_type_edge)
+			netdev_info(dev, "Operating a level interrupt chip with an edge interrupt.\n");
 	} else if (dev->irq) {
 		err = request_irq(dev->irq, m_can_isr, IRQF_SHARED, dev->name,
 				  dev);
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index 3a9edc292593..17de56056352 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -99,6 +99,7 @@ struct m_can_classdev {
 	int pm_clock_support;
 	int pm_wake_source;
 	int is_peripheral;
+	bool irq_type_edge;
 
 	// Cached M_CAN_IE register content
 	u32 active_interrupts;
-- 
2.45.2