RE: [PATCH v5 2/6] dmaengine: sh: rz-dmac: Move all CHCTRL updates under spinlock

[Biju Das]

Hi Claudiu,

> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@xxxxxxxxx>
> 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@xxxxxxxxx>
> >> Sent: 17 December 2025 13:52
> >> To: vkoul@xxxxxxxxxx; Fabrizio Castro
> >> <fabrizio.castro.jz@xxxxxxxxxxx>; Biju Das
> >> <biju.das.jz@xxxxxxxxxxxxxx>; geert+renesas@xxxxxxxxx; Prabhakar
> >> Mahadev Lad <prabhakar.mahadev- lad.rj@xxxxxxxxxxxxxx>
> >> Cc: Claudiu.Beznea <claudiu.beznea@xxxxxxxxx>;
> >> dmaengine@xxxxxxxxxxxxxxx; linux- kernel@xxxxxxxxxxxxxxx; Claudiu
> >> Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> >> Subject: [PATCH v5 2/6] dmaengine: sh: rz-dmac: Move all CHCTRL
> >> updates under spinlock
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >>
> >> 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