commit d887087c896881715c1a82f1d4f71fbfe5344ffd upstream.

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

This patch fixes the issue by passing a channel specific context
parameter instead of global one for the IRQ register and the 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@...>
Link: https://lore.kernel.org/all/20221025155657.1426948-3-biju.das.jz@bp.renesas.com
Cc: stable@...
[mkl: adjust commit message]
Signed-off-by: Marc Kleine-Budde <mkl@...>
[biju: fixed the conflicts manually]
Signed-off-by: Biju Das <biju.das.jz@...>
---
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 8acae71eb8bf..feba57833cdc 100644
--- a/drivers/net/can/rcar/rcar_canfd.c
+++ b/drivers/net/can/rcar/rcar_canfd.c
@@ -1195,11 +1195,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, RCANFD_NUM_CHANNELS)
- rcar_canfd_handle_channel_tx(gpriv, ch);
+ rcar_canfd_handle_channel_tx(priv->gpriv, priv->channel);

return IRQ_HANDLED;
}
@@ -1227,11 +1225,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, RCANFD_NUM_CHANNELS)
- rcar_canfd_handle_channel_err(gpriv, ch);
+ rcar_canfd_handle_channel_err(priv->gpriv, priv->channel);

return IRQ_HANDLED;
}
@@ -1649,6 +1645,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);

@@ -1677,7 +1674,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);
@@ -1691,7 +1688,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);
@@ -1715,7 +1712,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(ndev, &priv->napi, rcar_canfd_rx_poll,
--
2.25.1

Hi!

commit d887087c896881715c1a82f1d4f71fbfe5344ffd upstream.

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

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

Can you elaborate what "the issue" is? AFAICT the code worked before,
and will work after the patch, with maybe tiny little less overhead
after the patch?

Thanks and best regards,
Pavel

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, RCANFD_NUM_CHANNELS)
- rcar_canfd_handle_channel_tx(gpriv, ch);
+ rcar_canfd_handle_channel_tx(priv->gpriv, priv->channel);

return IRQ_HANDLED;
}
@@ -1227,11 +1225,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, RCANFD_NUM_CHANNELS)
- rcar_canfd_handle_channel_err(gpriv, ch);
+ rcar_canfd_handle_channel_err(priv->gpriv, priv->channel);

return IRQ_HANDLED;
}

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Subject: Re: [PATCH 5.10.y-cip] can: rcar_canfd: fix channel specific
IRQ handling for RZ/G2L

Hi!

commit d887087c896881715c1a82f1d4f71fbfe5344ffd upstream.

RZ/G2L has separate channel specific IRQs for transmit and error
interrupts. But the IRQ handler processes both channels, even if

there

no interrupt occurred on one of the channels.

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

interrupt.

Can you elaborate what "the issue" is? AFAICT the code worked before,
and will work after the patch, with maybe tiny little less overhead
after the patch?

Basically, tx IRQs are channel specific.
Previously if any txIRQ is triggered, you are unnecessarily
Checking IRQ status of the txIRQ which is not triggered.

For eg: if ch0 IRQ is triggered you are checking for ch1 IRQ status
as well. Similarly, if ch1 IRQ is triggered, then you are checking ch0
IRQ status.

Previous implementation is good for shared IRQ's like R-Car

Cheers,
Biju

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, RCANFD_NUM_CHANNELS)
- rcar_canfd_handle_channel_tx(gpriv, ch);
+ rcar_canfd_handle_channel_tx(priv->gpriv, priv->channel);

return IRQ_HANDLED;
}
@@ -1227,11 +1225,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, RCANFD_NUM_CHANNELS)
- rcar_canfd_handle_channel_err(gpriv, ch);
+ rcar_canfd_handle_channel_err(priv->gpriv, priv->channel);

return IRQ_HANDLED;
}

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hi!

RZ/G2L has separate channel specific IRQs for transmit and error
interrupts. But the IRQ handler processes both channels, even if

there

no interrupt occurred on one of the channels.

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

interrupt.

Can you elaborate what "the issue" is? AFAICT the code worked before,
and will work after the patch, with maybe tiny little less overhead
after the patch?

Basically, tx IRQs are channel specific.
Previously if any txIRQ is triggered, you are unnecessarily
Checking IRQ status of the txIRQ which is not triggered.

For eg: if ch0 IRQ is triggered you are checking for ch1 IRQ status
as well. Similarly, if ch1 IRQ is triggered, then you are checking ch0
IRQ status.

Understood, but checking ch1 status on ch0 IRQ is not going to cause
any problems, right?

Best regards,

Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Subject: Re: [PATCH 5.10.y-cip] can: rcar_canfd: fix channel specific
IRQ handling for RZ/G2L

Hi!

RZ/G2L has separate channel specific IRQs for transmit and error
interrupts. But the IRQ handler processes both channels, even if

there

no interrupt occurred on one of the channels.

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

interrupt.

Can you elaborate what "the issue" is? AFAICT the code worked
before, and will work after the patch, with maybe tiny little less
overhead after the patch?

Basically, tx IRQs are channel specific.
Previously if any txIRQ is triggered, you are unnecessarily Checking
IRQ status of the txIRQ which is not triggered.

For eg: if ch0 IRQ is triggered you are checking for ch1 IRQ status

as

well. Similarly, if ch1 IRQ is triggered, then you are checking ch0
IRQ status.

Understood, but checking ch1 status on ch0 IRQ is not going to cause
any problems, right?

IRQ processing should be fast as possible. You are improving latency of
the system by avoiding unnecessary checks and by calling an unwanted function call.

If it is shared IRQ, then it make sense you don't have any other alternative.
Since you have dedicated handler, make use of it and don't call any
Unnecessary function and checks in IRQ routine.


Cheers,
Biju

Hi!

For eg: if ch0 IRQ is triggered you are checking for ch1 IRQ status

as

well. Similarly, if ch1 IRQ is triggered, then you are checking ch0
IRQ status.

Understood, but checking ch1 status on ch0 IRQ is not going to cause
any problems, right?

IRQ processing should be fast as possible. You are improving latency of
the system by avoiding unnecessary checks and by calling an unwanted function call.

If it is shared IRQ, then it make sense you don't have any other alternative.
Since you have dedicated handler, make use of it and don't call any
Unnecessary function and checks in IRQ routine.

I understand it is just a performance tweak. Correct me if I'm wrong.

If there are no other comments and if testing passes I can apply the
patch.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Subject: Re: [PATCH 5.10.y-cip] can: rcar_canfd: fix channel specific
IRQ handling for RZ/G2L

Hi!

For eg: if ch0 IRQ is triggered you are checking for ch1 IRQ
status

as

well. Similarly, if ch1 IRQ is triggered, then you are checking
ch0 IRQ status.

Understood, but checking ch1 status on ch0 IRQ is not going to

cause

any problems, right?

IRQ processing should be fast as possible. You are improving latency
of the system by avoiding unnecessary checks and by calling an

unwanted function call.

If it is shared IRQ, then it make sense you don't have any other

alternative.

Since you have dedicated handler, make use of it and don't call any
Unnecessary function and checks in IRQ routine.

I understand it is just a performance tweak. Correct me if I'm wrong.

Yes, performance is one thing, also it will avoid unnecessary races
when you start handling IRQ's in both the cores.

Eg:- ch0 IRQ handler executed on core0 and ch1 IRQ handler on core 1, which
unnecessarily calls ch0 routine. See the races in rcar_canfd_tx_done().

Cheers,
Biju

If there are no other comments and if testing passes I can apply the
patch.

Hi!

commit d887087c896881715c1a82f1d4f71fbfe5344ffd upstream.

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

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

Thank you, applied.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany