[PATCH 0/3] R-Car CANFD fixes

[PATCH 0/3] R-Car CANFD fixes

[Biju Das]

This patch series fixes the below issues in R-Car CAN FD driver.

 1) Race condition in CAN driver under heavy CAN load condition
    with both channels enabled results in IRQ stom on global fifo
    receive irq line.
 2) Add channel specific tx interrupts handling for RZ/G2L SoC as it has
    separate IRQ lines for each tx.
 3) Remove unnecessary SoC specific checks in probe.

Biju Das (3):
  can: rcar_canfd: Fix IRQ storm on global fifo receive
  can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L
  can: rcar_canfd: Use devm_reset_control_get_optional_exclusive

 drivers/net/can/rcar/rcar_canfd.c | 50 +++++++++++++++++--------------
 1 file changed, 27 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Biju Das]

We are seeing IRQ storm on global receive IRQ line under heavy CAN
bus load conditions with both CAN channels are enabled.

Conditions:
  The global receive IRQ line is shared between can0 and can1, either
  of the channels can trigger interrupt while the other channel irq
  line is disabled(rfie).
  When global receive IRQ interrupt occurs, we mask the interrupt in
  irqhandler. Clearing and unmasking of the interrupt is happening in
  rx_poll(). There is a race condition where rx_poll unmask the
  interrupt, but the next irq handler does not mask the irq due to
  NAPIF_STATE_MISSED flag(for eg: can0 rx fifo interrupt enable is
  disabled and can1 is triggering rx interrupt, the delay in rx_poll()
  processing results in setting NAPIF_STATE_MISSED flag) leading to IRQ
  storm.

This patch fixes the issue by checking irq is masked or not in
irq handler and it masks the interrupt if it is unmasked.

Fixes: dd3bd23eb438 ("can: rcar_canfd: Add Renesas R-Car CAN FD driver")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 567620d215f8..1a3ae0b232c9 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1157,7 +1157,7 @@ static void rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u3
 {
 	struct rcar_canfd_channel *priv = gpriv->ch[ch];
 	u32 ridx = ch + RCANFD_RFFIFO_IDX;
-	u32 sts;
+	u32 sts, rfie;
 
 	/* Handle Rx interrupts */
 	sts = rcar_canfd_read(priv->base, RCANFD_RFSTS(gpriv, ridx));
@@ -1168,6 +1168,14 @@ static void rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u3
 					     RCANFD_RFCC(gpriv, ridx),
 					     RCANFD_RFCC_RFIE);
 			__napi_schedule(&priv->napi);
+		} else {
+			rfie = rcar_canfd_read(priv->base,
+					       RCANFD_RFCC(gpriv, ridx));
+			if (rfie & RCANFD_RFCC_RFIE)
+				/* Disable Rx FIFO interrupts */
+				rcar_canfd_clear_bit(priv->base,
+						     RCANFD_RFCC(gpriv, ridx),
+						     RCANFD_RFCC_RFIE);
 		}
 	}
 }
-- 
2.25.1

[PATCH 2/3] can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L

[Biju Das]

RZ/G2L has separate channel specific IRQs for transmit and error
interrupt. But the IRQ handler, process the code for both channels
even if there is no interrupt occurred on one of the channels.

This patch fixes the issue by passing channel specific context
parameter instead of global one for irq register and on irq handler,
it just handles the channel which is triggered the interrupt.

Fixes: 76e9353a80e9 ("can: rcar_canfd: Add support for RZ/G2L family")
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 1a3ae0b232c9..38d6a8f5691c 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1252,11 +1252,9 @@ static void rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32 ch
 
 static irqreturn_t rcar_canfd_channel_tx_interrupt(int irq, void *dev_id)
 {
-	struct rcar_canfd_global *gpriv = dev_id;
-	u32 ch;
+	struct rcar_canfd_channel *priv = dev_id;
 
-	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
-		rcar_canfd_handle_channel_tx(gpriv, ch);
+	rcar_canfd_handle_channel_tx(priv->gpriv, priv->channel);
 
 	return IRQ_HANDLED;
 }
@@ -1284,11 +1282,9 @@ static void rcar_canfd_handle_channel_err(struct rcar_canfd_global *gpriv, u32 c
 
 static irqreturn_t rcar_canfd_channel_err_interrupt(int irq, void *dev_id)
 {
-	struct rcar_canfd_global *gpriv = dev_id;
-	u32 ch;
+	struct rcar_canfd_channel *priv = dev_id;
 
-	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
-		rcar_canfd_handle_channel_err(gpriv, ch);
+	rcar_canfd_handle_channel_err(priv->gpriv, priv->channel);
 
 	return IRQ_HANDLED;
 }
@@ -1729,6 +1725,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 	priv->ndev = ndev;
 	priv->base = gpriv->base;
 	priv->channel = ch;
+	priv->gpriv = gpriv;
 	priv->can.clock.freq = fcan_freq;
 	dev_info(&pdev->dev, "can_clk rate is %u\n", priv->can.clock.freq);
 
@@ -1757,7 +1754,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 		}
 		err = devm_request_irq(&pdev->dev, err_irq,
 				       rcar_canfd_channel_err_interrupt, 0,
-				       irq_name, gpriv);
+				       irq_name, priv);
 		if (err) {
 			dev_err(&pdev->dev, "devm_request_irq CH Err(%d) failed, error %d\n",
 				err_irq, err);
@@ -1771,7 +1768,7 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 		}
 		err = devm_request_irq(&pdev->dev, tx_irq,
 				       rcar_canfd_channel_tx_interrupt, 0,
-				       irq_name, gpriv);
+				       irq_name, priv);
 		if (err) {
 			dev_err(&pdev->dev, "devm_request_irq Tx (%d) failed, error %d\n",
 				tx_irq, err);
@@ -1797,7 +1794,6 @@ static int rcar_canfd_channel_probe(struct rcar_canfd_global *gpriv, u32 ch,
 
 	priv->can.do_set_mode = rcar_canfd_do_set_mode;
 	priv->can.do_get_berr_counter = rcar_canfd_get_berr_counter;
-	priv->gpriv = gpriv;
 	SET_NETDEV_DEV(ndev, &pdev->dev);
 
 	netif_napi_add_weight(ndev, &priv->napi, rcar_canfd_rx_poll,
-- 
2.25.1

[PATCH 3/3] can: rcar_canfd: Use devm_reset_control_get_optional_exclusive

[Biju Das]

Replace devm_reset_control_get_exclusive->devm_reset_control_
get_optional_exclusive so that we can avoid unnecessary
SoC specific check in probe().

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/net/can/rcar/rcar_canfd.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
index 38d6a8f5691c..4b12ed85deca 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1893,17 +1893,17 @@ static int rcar_canfd_probe(struct platform_device *pdev)
 	gpriv->chip_id = chip_id;
 	gpriv->max_channels = max_channels;
 
-	if (gpriv->chip_id == RENESAS_RZG2L) {
-		gpriv->rstc1 = devm_reset_control_get_exclusive(&pdev->dev, "rstp_n");
-		if (IS_ERR(gpriv->rstc1))
-			return dev_err_probe(&pdev->dev, PTR_ERR(gpriv->rstc1),
-					     "failed to get rstp_n\n");
-
-		gpriv->rstc2 = devm_reset_control_get_exclusive(&pdev->dev, "rstc_n");
-		if (IS_ERR(gpriv->rstc2))
-			return dev_err_probe(&pdev->dev, PTR_ERR(gpriv->rstc2),
-					     "failed to get rstc_n\n");
-	}
+	gpriv->rstc1 = devm_reset_control_get_optional_exclusive(&pdev->dev,
+								 "rstp_n");
+	if (IS_ERR(gpriv->rstc1))
+		return dev_err_probe(&pdev->dev, PTR_ERR(gpriv->rstc1),
+				     "failed to get rstp_n\n");
+
+	gpriv->rstc2 = devm_reset_control_get_optional_exclusive(&pdev->dev,
+								 "rstc_n");
+	if (IS_ERR(gpriv->rstc2))
+		return dev_err_probe(&pdev->dev, PTR_ERR(gpriv->rstc2),
+				     "failed to get rstc_n\n");
 
 	/* Peripheral clock */
 	gpriv->clkp = devm_clk_get(&pdev->dev, "fck");
-- 
2.25.1

Re: [PATCH 0/3] R-Car CANFD fixes

[Marc Kleine-Budde]

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

On 22.10.2022 09:15:00, Biju Das wrote:
> This patch series fixes the below issues in R-Car CAN FD driver.
> 
>  1) Race condition in CAN driver under heavy CAN load condition
>     with both channels enabled results in IRQ stom on global fifo
                                                ^^^^ typo
>     receive irq line.
>  2) Add channel specific tx interrupts handling for RZ/G2L SoC as it has
>     separate IRQ lines for each tx.
>  3) Remove unnecessary SoC specific checks in probe.

Fixed typo while applying.

Thanks,
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 0/3] R-Car CANFD fixes

[Biju Das]

Hi Marc,

> Subject: Re: [PATCH 0/3] R-Car CANFD fixes
> 
> On 22.10.2022 09:15:00, Biju Das wrote:
> > This patch series fixes the below issues in R-Car CAN FD driver.
> >
> >  1) Race condition in CAN driver under heavy CAN load condition
> >     with both channels enabled results in IRQ stom on global fifo
>                                                 ^^^^ typo
> >     receive irq line.
> >  2) Add channel specific tx interrupts handling for RZ/G2L SoC as it has
> >     separate IRQ lines for each tx.
> >  3) Remove unnecessary SoC specific checks in probe.
> 
> Fixed typo while applying.

Thanks for fixing the typo.

Cheers,
Biju

Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Marc Kleine-Budde]

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

On 22.10.2022 09:15:01, Biju Das wrote:
> We are seeing IRQ storm on global receive IRQ line under heavy CAN
> bus load conditions with both CAN channels are enabled.
> 
> Conditions:
>   The global receive IRQ line is shared between can0 and can1, either
>   of the channels can trigger interrupt while the other channel irq
>   line is disabled(rfie).
>   When global receive IRQ interrupt occurs, we mask the interrupt in
>   irqhandler. Clearing and unmasking of the interrupt is happening in
>   rx_poll(). There is a race condition where rx_poll unmask the
>   interrupt, but the next irq handler does not mask the irq due to
>   NAPIF_STATE_MISSED flag.

Why does this happen? Is it a problem that you call
rcar_canfd_handle_global_receive() for a channel that has the IRQs
actually disabled in hardware?

>   (for eg: can0 rx fifo interrupt enable is
>   disabled and can1 is triggering rx interrupt, the delay in rx_poll()
>   processing results in setting NAPIF_STATE_MISSED flag) leading to IRQ
>   storm.
> 
> This patch fixes the issue by checking irq is masked or not in
> irq handler and it masks the interrupt if it is unmasked.

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 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Marc Kleine-Budde]

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

On 24.10.2022 17:37:35, Marc Kleine-Budde wrote:
> On 22.10.2022 09:15:01, Biju Das wrote:
> > We are seeing IRQ storm on global receive IRQ line under heavy CAN
> > bus load conditions with both CAN channels are enabled.
> > 
> > Conditions:
> >   The global receive IRQ line is shared between can0 and can1, either
> >   of the channels can trigger interrupt while the other channel irq
> >   line is disabled(rfie).
> >   When global receive IRQ interrupt occurs, we mask the interrupt in
> >   irqhandler. Clearing and unmasking of the interrupt is happening in
> >   rx_poll(). There is a race condition where rx_poll unmask the
> >   interrupt, but the next irq handler does not mask the irq due to
> >   NAPIF_STATE_MISSED flag.
> 
> Why does this happen? Is it a problem that you call
> rcar_canfd_handle_global_receive() for a channel that has the IRQs
> actually disabled in hardware?

Can you check if the IRQ is active _and_ enabled before handling the IRQ
on a particular channel?

A more clearer approach would be to get rid of the global interrupt
handlers at all. If the hardware only given 1 IRQ line for more than 1
channel, the driver would register an IRQ handler for each channel (with
the shared attribute). The IRQ handler must check, if the IRQ is pending
and enabled. If not return IRQ_NONE, otherwise handle and return IRQ_HANDLED.

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 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Biju Das]

Hi Marc,

> Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo
> receive
> 
> On 22.10.2022 09:15:01, Biju Das wrote:
> > We are seeing IRQ storm on global receive IRQ line under heavy CAN
> bus
> > load conditions with both CAN channels are enabled.
> >
> > Conditions:
> >   The global receive IRQ line is shared between can0 and can1,
> either
> >   of the channels can trigger interrupt while the other channel irq
> >   line is disabled(rfie).
> >   When global receive IRQ interrupt occurs, we mask the interrupt in
> >   irqhandler. Clearing and unmasking of the interrupt is happening
> in
> >   rx_poll(). There is a race condition where rx_poll unmask the
> >   interrupt, but the next irq handler does not mask the irq due to
> >   NAPIF_STATE_MISSED flag.
> 
> Why does this happen?

It is due to race between rx_poll() and interrupt triggered by other
Channel.

> Is it a problem that you call
> rcar_canfd_handle_global_receive() for a channel that has the IRQs
> actually disabled in hardware?

Yes, Due to other channel triggering interrupt and executing
the same call for channel0 again. 

Scenario:
 Channel0 IRQ line is disabled because of RXFiFo ch0 status in IRQ
and it schedule NAPI call. Before executing rx_poll, you get another
interrupt due to channel1 IRQ. Since RXFifo status is still set,
it will call napi_sched_prep() and state become missed.

Assume rx_poll() called it clear and unmask the IRQ line. This
time we get an IRQ from Channel0, since the state is missed state,
the line will be unmasked and we get IRQ storm

Finally, It will be like this you have an interrupt, which is not cleared
Leading to IRQ storm.

Cheers,
Biju

RE: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Biju Das]

Hi Marc,
> Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo
> receive
> 
> On 24.10.2022 17:37:35, Marc Kleine-Budde wrote:
> > On 22.10.2022 09:15:01, Biju Das wrote:
> > > We are seeing IRQ storm on global receive IRQ line under heavy CAN
> > > bus load conditions with both CAN channels are enabled.
> > >
> > > Conditions:
> > >   The global receive IRQ line is shared between can0 and can1,
> either
> > >   of the channels can trigger interrupt while the other channel
> irq
> > >   line is disabled(rfie).
> > >   When global receive IRQ interrupt occurs, we mask the interrupt
> in
> > >   irqhandler. Clearing and unmasking of the interrupt is happening
> in
> > >   rx_poll(). There is a race condition where rx_poll unmask the
> > >   interrupt, but the next irq handler does not mask the irq due to
> > >   NAPIF_STATE_MISSED flag.
> >
> > Why does this happen? Is it a problem that you call
> > rcar_canfd_handle_global_receive() for a channel that has the IRQs
> > actually disabled in hardware?
> 
> Can you check if the IRQ is active _and_ enabled before handling the
> IRQ on a particular channel?

You mean IRQ handler or rx_poll()??

IRQ handler check the status and disable(mask) the IRQ line.
rx_poll() clears the status and enable(unmask) the IRQ line.

Status flag is set by HW while line is in disabled/enabled state.

Channel0 and channel1 has 2 IRQ lines within the IP which is ored together
to provide global receive interrupt(shared line).


> 
> A more clearer approach would be to get rid of the global interrupt
> handlers at all. If the hardware only given 1 IRQ line for more than 1
> channel, the driver would register an IRQ handler for each channel
> (with the shared attribute). The IRQ handler must check, if the IRQ is
> pending and enabled. If not return IRQ_NONE, otherwise handle and
> return IRQ_HANDLED.

That involves restructuring the IRQ handler altogether.

RZ/G2L has shared line for rx fifos {ch0 and ch1} -> 2 IRQ routine with shared attributes
R-Car SoCs has shared line for rx fifos {ch0 and ch1} and error interrupts->3 IRQ routines with shared attributes.
R-CarV3U SoCs has shared line for rx fifos {ch0 to ch8} and error interrupts->9 IRQ routines with shared attributes.

Yes, I can send follow up patches for migrating to shared interrupt handlers as enhancement.
Please let me know.

Cheers,
Biju

Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Marc Kleine-Budde]

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

On 24.10.2022 16:55:56, Biju Das wrote:
> Hi Marc,
> > Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo
> > receive
> > 
> > On 24.10.2022 17:37:35, Marc Kleine-Budde wrote:
> > > On 22.10.2022 09:15:01, Biju Das wrote:
> > > > We are seeing IRQ storm on global receive IRQ line under heavy CAN
> > > > bus load conditions with both CAN channels are enabled.
> > > >
> > > > Conditions:
> > > >   The global receive IRQ line is shared between can0 and can1,
> > either
> > > >   of the channels can trigger interrupt while the other channel
> > irq
> > > >   line is disabled(rfie).
> > > >   When global receive IRQ interrupt occurs, we mask the interrupt
> > in
> > > >   irqhandler. Clearing and unmasking of the interrupt is happening
> > in
> > > >   rx_poll(). There is a race condition where rx_poll unmask the
> > > >   interrupt, but the next irq handler does not mask the irq due to
> > > >   NAPIF_STATE_MISSED flag.
> > >
> > > Why does this happen? Is it a problem that you call
> > > rcar_canfd_handle_global_receive() for a channel that has the IRQs
> > > actually disabled in hardware?
> > 
> > Can you check if the IRQ is active _and_ enabled before handling the
> > IRQ on a particular channel?
> 
> You mean IRQ handler or rx_poll()??

I mean the IRQ handler.

Consider the IRQ for channel0 is disabled but active and the IRQ for
channel1 is enabled and active. The
rcar_canfd_global_receive_fifo_interrupt() will iterate over both
channels, and rcar_canfd_handle_global_receive() will serve the channel0
IRQ, even if the IRQ is _not_ enabled. So I suggested to only handle a
channel's RX IRQ if that IRQ is actually enabled.

Assuming "cc & RCANFD_RFCC_RFI" checks if IRQ is enabled:

index 567620d215f8..ea828c1bd3a1 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1157,11 +1157,13 @@ static void rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u3
 {
        struct rcar_canfd_channel *priv = gpriv->ch[ch];
        u32 ridx = ch + RCANFD_RFFIFO_IDX;
-       u32 sts;
+       u32 sts, cc;
 
        /* Handle Rx interrupts */
        sts = rcar_canfd_read(priv->base, RCANFD_RFSTS(gpriv, ridx));
-       if (likely(sts & RCANFD_RFSTS_RFIF)) {
+       cc = rcar_canfd_read(priv->base, RCANFD_RFCC(gpriv, ridx));
+       if (likely(sts & RCANFD_RFSTS_RFIF &&
+                  cc & RCANFD_RFCC_RFIE)) {
                if (napi_schedule_prep(&priv->napi)) {
                        /* Disable Rx FIFO interrupts */
                        rcar_canfd_clear_bit(priv->base,

Please check if that fixes your issue.

> IRQ handler check the status and disable(mask) the IRQ line.
> rx_poll() clears the status and enable(unmask) the IRQ line.
> 
> Status flag is set by HW while line is in disabled/enabled state.
> 
> Channel0 and channel1 has 2 IRQ lines within the IP which is ored together
> to provide global receive interrupt(shared line).

> > A more clearer approach would be to get rid of the global interrupt
> > handlers at all. If the hardware only given 1 IRQ line for more than 1
> > channel, the driver would register an IRQ handler for each channel
> > (with the shared attribute). The IRQ handler must check, if the IRQ is
                     ^^^^^^^^^
That should be "flag".

> > pending and enabled. If not return IRQ_NONE, otherwise handle and
> > return IRQ_HANDLED.
> 
> That involves restructuring the IRQ handler altogether.

ACK

> RZ/G2L has shared line for rx fifos {ch0 and ch1} -> 2 IRQ routine
> with shared attributes.

It's the same IRQ handler (or IRQ routine), but called 1x for each
channel, so 2x in total. The SHARED is actually a IRQ flag in the 4th
argument in the devm_request_irq() function.

| devm_request_irq(..., ..., ..., IRQF_SHARED, ..., ...);

> R-Car SoCs has shared line for rx fifos {ch0 and ch1} and error
> interrupts->3 IRQ routines with shared attributes.

> R-CarV3U SoCs has shared line for rx fifos {ch0 to ch8} and error
> interrupts->9 IRQ routines with shared attributes.

I think you got the point, I just wanted to point out the usual way they
are called.

> Yes, I can send follow up patches for migrating to shared interrupt
> handlers as enhancement. Please let me know.

Please check if my patch snippet from above works. To fix the IRQ storm
problem I'd like to have a simple and short solution that can go into
stable before restructuring the IRQ handlers.

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 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Biju Das]

Hi Marc,

> Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo
> receive
> 
> On 24.10.2022 16:55:56, Biju Das wrote:
> > Hi Marc,
> > > Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global
> > > fifo receive
> > >
> > > On 24.10.2022 17:37:35, Marc Kleine-Budde wrote:
> > > > On 22.10.2022 09:15:01, Biju Das wrote:
> > > > > We are seeing IRQ storm on global receive IRQ line under heavy
> > > > > CAN bus load conditions with both CAN channels are enabled.
> > > > >
> > > > > Conditions:
> > > > >   The global receive IRQ line is shared between can0 and can1,
> > > either
> > > > >   of the channels can trigger interrupt while the other
> channel
> > > irq
> > > > >   line is disabled(rfie).
> > > > >   When global receive IRQ interrupt occurs, we mask the
> > > > > interrupt
> > > in
> > > > >   irqhandler. Clearing and unmasking of the interrupt is
> > > > > happening
> > > in
> > > > >   rx_poll(). There is a race condition where rx_poll unmask
> the
> > > > >   interrupt, but the next irq handler does not mask the irq
> due to
> > > > >   NAPIF_STATE_MISSED flag.
> > > >
> > > > Why does this happen? Is it a problem that you call
> > > > rcar_canfd_handle_global_receive() for a channel that has the
> IRQs
> > > > actually disabled in hardware?
> > >
> > > Can you check if the IRQ is active _and_ enabled before handling
> the
> > > IRQ on a particular channel?
> >
> > You mean IRQ handler or rx_poll()??
> 
> I mean the IRQ handler.
> 
> Consider the IRQ for channel0 is disabled but active and the IRQ for
> channel1 is enabled and active. The
> rcar_canfd_global_receive_fifo_interrupt() will iterate over both
> channels, and rcar_canfd_handle_global_receive() will serve the
> channel0 IRQ, even if the IRQ is _not_ enabled. So I suggested to only
> handle a channel's RX IRQ if that IRQ is actually enabled.
> 
> Assuming "cc & RCANFD_RFCC_RFI" checks if IRQ is enabled:


> 
> index 567620d215f8..ea828c1bd3a1 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -1157,11 +1157,13 @@ static void
> rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u3
> {
>         struct rcar_canfd_channel *priv = gpriv->ch[ch];
>         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> -       u32 sts;
> +       u32 sts, cc;
> 
>         /* Handle Rx interrupts */
>         sts = rcar_canfd_read(priv->base, RCANFD_RFSTS(gpriv, ridx));
> -       if (likely(sts & RCANFD_RFSTS_RFIF)) {
> +       cc = rcar_canfd_read(priv->base, RCANFD_RFCC(gpriv, ridx));
> +       if (likely(sts & RCANFD_RFSTS_RFIF &&
> +                  cc & RCANFD_RFCC_RFIE)) {
>                 if (napi_schedule_prep(&priv->napi)) {
>                         /* Disable Rx FIFO interrupts */
>                         rcar_canfd_clear_bit(priv->base,
> 
> Please check if that fixes your issue.

Looks like your solution also will work.

Tomorrow will check and provide you feedback.

> 
> > IRQ handler check the status and disable(mask) the IRQ line.
> > rx_poll() clears the status and enable(unmask) the IRQ line.
> >
> > Status flag is set by HW while line is in disabled/enabled state.
> >
> > Channel0 and channel1 has 2 IRQ lines within the IP which is ored
> > together to provide global receive interrupt(shared line).
> 
> > > A more clearer approach would be to get rid of the global
> interrupt
> > > handlers at all. If the hardware only given 1 IRQ line for more
> than
> > > 1 channel, the driver would register an IRQ handler for each
> channel
> > > (with the shared attribute). The IRQ handler must check, if the
> IRQ
> > > is
>                      ^^^^^^^^^
> That should be "flag".
OK.

> 
> > > pending and enabled. If not return IRQ_NONE, otherwise handle and
> > > return IRQ_HANDLED.
> >
> > That involves restructuring the IRQ handler altogether.
> 
> ACK
> 
> > RZ/G2L has shared line for rx fifos {ch0 and ch1} -> 2 IRQ routine
> > with shared attributes.
> 
> It's the same IRQ handler (or IRQ routine), but called 1x for each
> channel, so 2x in total. The SHARED is actually a IRQ flag in the 4th
> argument in the devm_request_irq() function.
> 
> | devm_request_irq(..., ..., ..., IRQF_SHARED, ..., ...);
> 
> > R-Car SoCs has shared line for rx fifos {ch0 and ch1} and error
> > interrupts->3 IRQ routines with shared attributes.
> 
> > R-CarV3U SoCs has shared line for rx fifos {ch0 to ch8} and error
> > interrupts->9 IRQ routines with shared attributes.
> 
> I think you got the point, I just wanted to point out the usual way
> they are called.
> 
> > Yes, I can send follow up patches for migrating to shared interrupt
> > handlers as enhancement. Please let me know.
> 
> Please check if my patch snippet from above works. To fix the IRQ
> storm problem I'd like to have a simple and short solution that can go
> into stable before restructuring the IRQ handlers.

OK, Tomorrow will provide you the feedback.

Cheers,
Biju

RE: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Biju Das]

Hi Marc,

> Subject: RE: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo
> receive
> 
> Hi Marc,
> 
> > Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global
> fifo
> > receive
> >
> > On 24.10.2022 16:55:56, Biju Das wrote:
> > > Hi Marc,
> > > > Subject: Re: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on
> global
> > > > fifo receive
> > > >
> > > > On 24.10.2022 17:37:35, Marc Kleine-Budde wrote:
> > > > > On 22.10.2022 09:15:01, Biju Das wrote:
> > > > > > We are seeing IRQ storm on global receive IRQ line under
> heavy
> > > > > > CAN bus load conditions with both CAN channels are enabled.
> > > > > >
> > > > > > Conditions:
> > > > > >   The global receive IRQ line is shared between can0 and
> can1,
> > > > either
> > > > > >   of the channels can trigger interrupt while the other
> > channel
> > > > irq
> > > > > >   line is disabled(rfie).
> > > > > >   When global receive IRQ interrupt occurs, we mask the
> > > > > > interrupt
> > > > in
> > > > > >   irqhandler. Clearing and unmasking of the interrupt is
> > > > > > happening
> > > > in
> > > > > >   rx_poll(). There is a race condition where rx_poll unmask
> > the
> > > > > >   interrupt, but the next irq handler does not mask the irq
> > due to
> > > > > >   NAPIF_STATE_MISSED flag.
> > > > >
> > > > > Why does this happen? Is it a problem that you call
> > > > > rcar_canfd_handle_global_receive() for a channel that has the
> > IRQs
> > > > > actually disabled in hardware?
> > > >
> > > > Can you check if the IRQ is active _and_ enabled before handling
> > the
> > > > IRQ on a particular channel?
> > >
> > > You mean IRQ handler or rx_poll()??
> >
> > I mean the IRQ handler.
> >
> > Consider the IRQ for channel0 is disabled but active and the IRQ for
> > channel1 is enabled and active. The
> > rcar_canfd_global_receive_fifo_interrupt() will iterate over both
> > channels, and rcar_canfd_handle_global_receive() will serve the
> > channel0 IRQ, even if the IRQ is _not_ enabled. So I suggested to
> only
> > handle a channel's RX IRQ if that IRQ is actually enabled.
> >
> > Assuming "cc & RCANFD_RFCC_RFI" checks if IRQ is enabled:
> 
> 
> >
> > index 567620d215f8..ea828c1bd3a1 100644
> > --- a/drivers/net/can/rcar/rcar_canfd.c
> > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > @@ -1157,11 +1157,13 @@ static void
> > rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u3
> {
> >         struct rcar_canfd_channel *priv = gpriv->ch[ch];
> >         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> > -       u32 sts;
> > +       u32 sts, cc;
> >
> >         /* Handle Rx interrupts */
> >         sts = rcar_canfd_read(priv->base, RCANFD_RFSTS(gpriv,
> ridx));
> > -       if (likely(sts & RCANFD_RFSTS_RFIF)) {
> > +       cc = rcar_canfd_read(priv->base, RCANFD_RFCC(gpriv, ridx));
> > +       if (likely(sts & RCANFD_RFSTS_RFIF &&
> > +                  cc & RCANFD_RFCC_RFIE)) {
> >                 if (napi_schedule_prep(&priv->napi)) {
> >                         /* Disable Rx FIFO interrupts */
> >                         rcar_canfd_clear_bit(priv->base,
> >
> > Please check if that fixes your issue.

Yes, it fixes the issue.

> 
> >
> > > IRQ handler check the status and disable(mask) the IRQ line.
> > > rx_poll() clears the status and enable(unmask) the IRQ line.
> > >
> > > Status flag is set by HW while line is in disabled/enabled state.
> > >
> > > Channel0 and channel1 has 2 IRQ lines within the IP which is ored
> > > together to provide global receive interrupt(shared line).
> >
> > > > A more clearer approach would be to get rid of the global
> > interrupt
> > > > handlers at all. If the hardware only given 1 IRQ line for more
> > than
> > > > 1 channel, the driver would register an IRQ handler for each
> > channel
> > > > (with the shared attribute). The IRQ handler must check, if the
> > IRQ
> > > > is
> >                      ^^^^^^^^^
> > That should be "flag".
> OK.
> 
> >
> > > > pending and enabled. If not return IRQ_NONE, otherwise handle
> and
> > > > return IRQ_HANDLED.
> > >
> > > That involves restructuring the IRQ handler altogether.
> >
> > ACK
> >
> > > RZ/G2L has shared line for rx fifos {ch0 and ch1} -> 2 IRQ routine
> > > with shared attributes.
> >
> > It's the same IRQ handler (or IRQ routine), but called 1x for each
> > channel, so 2x in total. The SHARED is actually a IRQ flag in the
> 4th
> > argument in the devm_request_irq() function.
> >
> > | devm_request_irq(..., ..., ..., IRQF_SHARED, ..., ...);
> >
> > > R-Car SoCs has shared line for rx fifos {ch0 and ch1} and error
> > > interrupts->3 IRQ routines with shared attributes.
> >
> > > R-CarV3U SoCs has shared line for rx fifos {ch0 to ch8} and error
> > > interrupts->9 IRQ routines with shared attributes.
> >
> > I think you got the point, I just wanted to point out the usual way
> > they are called.
> >
> > > Yes, I can send follow up patches for migrating to shared
> interrupt
> > > handlers as enhancement. Please let me know.
> >
> > Please check if my patch snippet from above works. To fix the IRQ
> > storm problem I'd like to have a simple and short solution that can
> go
> > into stable before restructuring the IRQ handlers.
> 
> OK, Tomorrow will provide you the feedback.

I will send V2 with these changes.

Cheers,
Biju

Re: RE: [PATCH 1/3] can: rcar_canfd: Fix IRQ storm on global fifo receive

[Marc Kleine-Budde]

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

On 25.10.2022 15:50:18, Biju Das wrote:
[...]
> > > index 567620d215f8..ea828c1bd3a1 100644
> > > --- a/drivers/net/can/rcar/rcar_canfd.c
> > > +++ b/drivers/net/can/rcar/rcar_canfd.c
> > > @@ -1157,11 +1157,13 @@ static void
> > > rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u3
> > {
> > >         struct rcar_canfd_channel *priv = gpriv->ch[ch];
> > >         u32 ridx = ch + RCANFD_RFFIFO_IDX;
> > > -       u32 sts;
> > > +       u32 sts, cc;
> > >
> > >         /* Handle Rx interrupts */
> > >         sts = rcar_canfd_read(priv->base, RCANFD_RFSTS(gpriv,
> > ridx));
> > > -       if (likely(sts & RCANFD_RFSTS_RFIF)) {
> > > +       cc = rcar_canfd_read(priv->base, RCANFD_RFCC(gpriv, ridx));
> > > +       if (likely(sts & RCANFD_RFSTS_RFIF &&
> > > +                  cc & RCANFD_RFCC_RFIE)) {
> > >                 if (napi_schedule_prep(&priv->napi)) {
> > >                         /* Disable Rx FIFO interrupts */
> > >                         rcar_canfd_clear_bit(priv->base,
> > >
> > > Please check if that fixes your issue.
> 
> Yes, it fixes the issue.

\o/

> I will send V2 with these changes.

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