From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Both rz_dmac_disable_hw() and rz_dmac_irq_handle_channel() update the CHCTRL registers. To avoid concurrency issues when updating these registers, take the virtual channel lock. All other CHCTRL updates were already protected by the same lock.
Previously, rz_dmac_disable_hw() disabled and re-enabled local IRQs, before accessing CHCTRL registers but this does not ensure race-free access. Remove the local IRQ disable/enable code as well.
Fixes: 5000d37042a6 ("dmaengine: sh: Add DMAC driver for RZ/G2L SoC") Cc: stable@vger.kernel.org Signed-off-by: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com ---
Changes in v5: - none, this patch is new
drivers/dma/sh/rz-dmac.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c index c8e3d9f77b8a..c013bf30fa5e 100644 --- a/drivers/dma/sh/rz-dmac.c +++ b/drivers/dma/sh/rz-dmac.c @@ -298,13 +298,10 @@ static void rz_dmac_disable_hw(struct rz_dmac_chan *channel) { struct dma_chan *chan = &channel->vc.chan; struct rz_dmac *dmac = to_rz_dmac(chan->device); - unsigned long flags;
dev_dbg(dmac->dev, "%s channel %d\n", __func__, channel->index);
- local_irq_save(flags); rz_dmac_ch_writel(channel, CHCTRL_DEFAULT, CHCTRL, 1); - local_irq_restore(flags); }
static void rz_dmac_set_dmars_register(struct rz_dmac *dmac, int nr, u32 dmars) @@ -569,8 +566,8 @@ static int rz_dmac_terminate_all(struct dma_chan *chan) unsigned int i; LIST_HEAD(head);
- rz_dmac_disable_hw(channel); spin_lock_irqsave(&channel->vc.lock, flags); + rz_dmac_disable_hw(channel); for (i = 0; i < DMAC_NR_LMDESC; i++) lmdesc[i].header = 0;
@@ -721,6 +718,8 @@ static irqreturn_t rz_dmac_irq_handler(int irq, void *dev_id) { struct rz_dmac_chan *channel = dev_id;
+ guard(spinlock)(&channel->vc.lock); + if (channel) { rz_dmac_irq_handle_channel(channel); return IRQ_WAKE_THREAD;
Hi, Biju,
On 12/17/25 18:16, Biju Das wrote:
Hi Claudiu,
-----Original Message----- From: Claudiu claudiu.beznea@tuxon.dev Sent: 17 December 2025 13:52 To: vkoul@kernel.org; Fabrizio Castro fabrizio.castro.jz@renesas.com; Biju Das biju.das.jz@bp.renesas.com; geert+renesas@glider.be; Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com> Cc: Claudiu.Beznea claudiu.beznea@tuxon.dev; dmaengine@vger.kernel.org; linux- kernel@vger.kernel.org; Claudiu Beznea claudiu.beznea.uj@bp.renesas.com; stable@vger.kernel.org Subject: [PATCH v5 2/6] dmaengine: sh: rz-dmac: Move all CHCTRL updates under spinlock
From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Both rz_dmac_disable_hw() and rz_dmac_irq_handle_channel() update the CHCTRL registers. To avoid concurrency issues when updating these registers, take the virtual channel lock. All other CHCTRL updates were already protected by the same lock.
Previously, rz_dmac_disable_hw() disabled and re-enabled local IRQs, before accessing CHCTRL registers but this does not ensure race-free access.
Maybe I am missing some thing here about race-access:
local_irq_save(flags); rz_dmac_ch_writel(channel, CHCTRL_DEFAULT, CHCTRL, 1);
After local_irq_save there won't be any IRQ. So how there can be a race in IRQ handler.
My point was to address races that may happen b/w different cores trying to set CHCTRL. E.g.:
core0: take the IRQ and set CHCTRL core1: call rz_dmac_issue_pending() -> rz_dmac_xfer_desc() -> rz_dmac_enable_hw() -> set CHCTRL
However, looking again though the HW manual, the CHCTRL returns zero when it is read, for each individual bit. Thus, there is no need for any kind of locking around this register. Also, read-modify-write approach when updating settings though it is not needed.
I'll adjust it in the next version.
Thank you, Claudiu
Hi Claudiu,
-----Original Message----- From: Claudiu Beznea claudiu.beznea@tuxon.dev Sent: 18 December 2025 07:44 Subject: Re: [PATCH v5 2/6] dmaengine: sh: rz-dmac: Move all CHCTRL updates under spinlock
Hi, Biju,
On 12/17/25 18:16, Biju Das wrote:
Hi Claudiu,
-----Original Message----- From: Claudiu claudiu.beznea@tuxon.dev Sent: 17 December 2025 13:52 To: vkoul@kernel.org; Fabrizio Castro fabrizio.castro.jz@renesas.com; Biju Das biju.das.jz@bp.renesas.com; geert+renesas@glider.be; Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com> Cc: Claudiu.Beznea claudiu.beznea@tuxon.dev; dmaengine@vger.kernel.org; linux- kernel@vger.kernel.org; Claudiu Beznea claudiu.beznea.uj@bp.renesas.com; stable@vger.kernel.org Subject: [PATCH v5 2/6] dmaengine: sh: rz-dmac: Move all CHCTRL updates under spinlock
From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Both rz_dmac_disable_hw() and rz_dmac_irq_handle_channel() update the CHCTRL registers. To avoid concurrency issues when updating these registers, take the virtual channel lock. All other CHCTRL updates were already protected by the
same lock.
Previously, rz_dmac_disable_hw() disabled and re-enabled local IRQs, before accessing CHCTRL registers but this does not ensure race-free access.
Maybe I am missing some thing here about race-access:
local_irq_save(flags); rz_dmac_ch_writel(channel, CHCTRL_DEFAULT, CHCTRL, 1);
After local_irq_save there won't be any IRQ. So how there can be a race in IRQ handler.
My point was to address races that may happen b/w different cores trying to set CHCTRL. E.g.:
core0: take the IRQ and set CHCTRL core1: call rz_dmac_issue_pending() -> rz_dmac_xfer_desc() -> rz_dmac_enable_hw() -> set CHCTRL
For that you could use a new lock. Currently all the calls are serialized using vc_lock In irqhandler and irqhandler unnecessarily keeps spinning as the chctrl register is the only shared Data in interrupt handler.
However, looking again though the HW manual, the CHCTRL returns zero when it is read, for each individual bit. Thus, there is no need for any kind of locking around this register. Also, read- modify-write approach when updating settings though it is not needed.
But still you need a lock other than vc_lock as CHCTRL is shared register between IRQhandler and tasklet/process context
Cheers, Biju
Hi Claudiu,
-----Original Message----- From: Claudiu claudiu.beznea@tuxon.dev Sent: 17 December 2025 13:52 To: vkoul@kernel.org; Fabrizio Castro fabrizio.castro.jz@renesas.com; Biju Das biju.das.jz@bp.renesas.com; geert+renesas@glider.be; Prabhakar Mahadev Lad <prabhakar.mahadev- lad.rj@bp.renesas.com> Cc: Claudiu.Beznea claudiu.beznea@tuxon.dev; dmaengine@vger.kernel.org; linux- kernel@vger.kernel.org; Claudiu Beznea claudiu.beznea.uj@bp.renesas.com; stable@vger.kernel.org Subject: [PATCH v5 2/6] dmaengine: sh: rz-dmac: Move all CHCTRL updates under spinlock
From: Claudiu Beznea claudiu.beznea.uj@bp.renesas.com
Both rz_dmac_disable_hw() and rz_dmac_irq_handle_channel() update the CHCTRL registers. To avoid concurrency issues when updating these registers, take the virtual channel lock. All other CHCTRL updates were already protected by the same lock.
Previously, rz_dmac_disable_hw() disabled and re-enabled local IRQs, before accessing CHCTRL registers but this does not ensure race-free access.
Maybe I am missing some thing here about race-access:
local_irq_save(flags); rz_dmac_ch_writel(channel, CHCTRL_DEFAULT, CHCTRL, 1);
After local_irq_save there won't be any IRQ. So how there can be a race in IRQ handler.
rz_dmac_irq_handle_channel(){
rz_dmac_ch_writel(channel, chctrl | CHCTRL_CLREND, CHCTRL, 1); }
Now adding a vc lock inside interrupt handler the IRQ routine is serialized
Cheers, Biju
linux-stable-mirror@lists.linaro.org
-
Biju Das -
Claudiu -
Claudiu Beznea