[PATCH v2 0/3] R-Car CANFD fixes

[PATCH v2 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 storm 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.

v1->v2:
 * Added check for irq active and enabled before handling the irq on a
   particular channel.

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 | 46 +++++++++++++++----------------
 1 file changed, 22 insertions(+), 24 deletions(-)

-- 
2.25.1

[PATCH v2 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 active and enabled before
handling the irq on a particular channel.

Fixes: dd3bd23eb438 ("can: rcar_canfd: Add Renesas R-Car CAN FD driver")
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Added check for irq active and enabled before handling the irq on a
   particular channel.
---
 drivers/net/can/rcar/rcar_canfd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
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,
-- 
2.25.1

[PATCH v2 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>
---
v1->v2:
 * No change
---
 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 ea828c1bd3a1..198da643ee6d 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1246,11 +1246,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;
 }
@@ -1278,11 +1276,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;
 }
@@ -1723,6 +1719,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);
 
@@ -1751,7 +1748,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);
@@ -1765,7 +1762,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);
@@ -1791,7 +1788,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 v2 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>
---
v1->v2:
 * No change.
---
 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 198da643ee6d..a0dd6044830b 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1887,17 +1887,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 v2 2/3] can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L

[Marc Kleine-Budde]

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

On 25.10.2022 16:56:56, Biju Das wrote:
> 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.

Please clean up signatures of the IRQ handlers you touch, it's a little
mess. Change:

| rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32 ch)

to:

| rcar_canfd_handle_channel_tx(struct rcar_canfd_channel *priv)

Same for:

| static void rcar_canfd_handle_channel_err(struct rcar_canfd_global *gpriv, u32 ch)



In a separate patch, please clean up these, too:

| static void rcar_canfd_handle_global_err(struct rcar_canfd_global *gpriv, u32 ch)
| static void rcar_canfd_handle_global_receive(struct rcar_canfd_global *gpriv, u32 ch)
| static void rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch)

Why are 2 of the above functions called "global" as they work on a
specific channel? That can be streamlined, too.

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 v2 3/3] can: rcar_canfd: Use devm_reset_control_get_optional_exclusive

[Geert Uytterhoeven]

On Tue, Oct 25, 2022 at 6:03 PM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> 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>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Marc Kleine-Budde]

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

On 25.10.2022 16:56:57, Biju Das wrote:
> 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>

Applied this patch to linux-can-next/main only. The other will go into
can/main after final review.

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 v2 2/3] can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L

[Biju Das]

Hi Marc,

Thanks for the review.

> Subject: Re: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ
> handling for RZ/G2L
> 
> On 25.10.2022 16:56:56, Biju Das wrote:
> > 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.
> 
> Please clean up signatures of the IRQ handlers you touch, it's a
> little mess. Change:
> 
> | rcar_canfd_handle_channel_tx(struct rcar_canfd_global *gpriv, u32
> ch)
> 
> to:
> 
> | rcar_canfd_handle_channel_tx(struct rcar_canfd_channel *priv)
> 
> Same for:
> 
> | static void rcar_canfd_handle_channel_err(struct rcar_canfd_global
> | *gpriv, u32 ch)
> 

OK.

> 
> 
> In a separate patch, please clean up these, too:
> 
> | static void rcar_canfd_handle_global_err(struct rcar_canfd_global
> | *gpriv, u32 ch) static void rcar_canfd_handle_global_receive(struct
> | rcar_canfd_global *gpriv, u32 ch) static void
> | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch)
> 
> Why are 2 of the above functions called "global" as they work on a
> specific channel? That can be streamlined, too.
> 

The function name is as per the hardware manual, Interrupt sources are classified into global and channel interrupts.

• Global interrupts (2 sources):
— Receive FIFO interrupt
— Global error interrupt
• Channel interrupts (3 sources/channel):

Maybe we could change "rcar_canfd_handle_global_receive"->"rcar_canfd_handle_channel_receive", as from driver point
It is not global anymore?? Please let me know.

Cheers,
Biju

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

[Marc Kleine-Budde]

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

On 26.10.2022 09:34:41, Biju Das wrote:
> > In a separate patch, please clean up these, too:
> > 
> > | static void rcar_canfd_handle_global_err(struct rcar_canfd_global
> > | *gpriv, u32 ch) static void rcar_canfd_handle_global_receive(struct
> > | rcar_canfd_global *gpriv, u32 ch) static void
> > | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32 ch)
> > 
> > Why are 2 of the above functions called "global" as they work on a
> > specific channel? That can be streamlined, too.
> > 
> 
> The function name is as per the hardware manual, Interrupt sources are
> classified into global and channel interrupts.
> 
> • Global interrupts (2 sources):
> — Receive FIFO interrupt
> — Global error interrupt
> • Channel interrupts (3 sources/channel):

I see. Keep the functions as is.

> Maybe we could change
> "rcar_canfd_handle_global_receive"->"rcar_canfd_handle_channel_receive",
> as from driver point It is not global anymore?? Please let me know.

Never mind - the gpriv and channel numbers are needed sometimes even in
the functions working on a single channel. Never mind. I'll take patches
1 and 2 as they are.

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 v2 2/3] can: rcar_canfd: Fix channel specific IRQ handling for RZ/G2L

[Biju Das]

Hi Marc,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/3] can: rcar_canfd: Fix channel specific IRQ
> handling for RZ/G2L
> 
> On 26.10.2022 09:34:41, Biju Das wrote:
> > > In a separate patch, please clean up these, too:
> > >
> > > | static void rcar_canfd_handle_global_err(struct
> rcar_canfd_global
> > > | *gpriv, u32 ch) static void
> > > | rcar_canfd_handle_global_receive(struct
> > > | rcar_canfd_global *gpriv, u32 ch) static void
> > > | rcar_canfd_channel_remove(struct rcar_canfd_global *gpriv, u32
> ch)
> > >
> > > Why are 2 of the above functions called "global" as they work on a
> > > specific channel? That can be streamlined, too.
> > >
> >
> > The function name is as per the hardware manual, Interrupt sources
> are
> > classified into global and channel interrupts.
> >
> > • Global interrupts (2 sources):
> > — Receive FIFO interrupt
> > — Global error interrupt
> > • Channel interrupts (3 sources/channel):
> 
> I see. Keep the functions as is.
> 
> > Maybe we could change
> > "rcar_canfd_handle_global_receive"-
> >"rcar_canfd_handle_channel_receive
> > ", as from driver point It is not global anymore?? Please let me
> know.
> 
> Never mind - the gpriv and channel numbers are needed sometimes even
> in the functions working on a single channel. Never mind. I'll take
> patches
> 1 and 2 as they are.

OK, Thanks.

Cheers,
Biju