| Message ID | 20221102190200.1166569-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Pavel Machek |
| Headers | show |
| Series | [5.10.y-cip] can: rcar_canfd: fix channel specific IRQ handling for RZ/G2L | expand |
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; > }
> 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
> 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
> 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
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,
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@bp.renesas.com> Link: https://lore.kernel.org/all/20221025155657.1426948-3-biju.das.jz@bp.renesas.com Cc: stable@vger.kernel.org [mkl: adjust commit message] Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> [biju: fixed the conflicts manually] 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(-)