| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<TY3PR01MB11346E51F609DDEC61E13609B8684A@TY3PR01MB11346.jpnprd01.prod.outlook.com>
Date: Wed, 7 Jan 2026 13:48:04 +0000
From: Biju Das <biju.das.jz@...renesas.com>
To: Claudiu.Beznea <claudiu.beznea@...on.dev>, "vkoul@...nel.org"
<vkoul@...nel.org>, Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
"geert+renesas@...der.be" <geert+renesas@...der.be>, Prabhakar Mahadev Lad
<prabhakar.mahadev-lad.rj@...renesas.com>
CC: "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Claudiu Beznea
<claudiu.beznea.uj@...renesas.com>
Subject: RE: [PATCH v6 8/8] dmaengine: sh: rz-dmac: Add
device_{pause,resume}() callbacks
Hi Claudiu,
> -----Original Message-----
> From: Claudiu Beznea <claudiu.beznea@...on.dev>
> Sent: 07 January 2026 13:18
> Subject: Re: [PATCH v6 8/8] dmaengine: sh: rz-dmac: Add device_{pause,resume}() callbacks
>
> Hi, Biju,
>
> On 12/23/25 16:43, Biju Das wrote:
> > Hi Claudiu,
> >
> >> -----Original Message-----
> >> From: Claudiu <claudiu.beznea@...on.dev>
> >> Sent: 23 December 2025 13:50
> >> Subject: [PATCH v6 8/8] dmaengine: sh: rz-dmac: Add
> >> device_{pause,resume}() callbacks
> >>
> >> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >>
> >> Add support for device_{pause, resume}() callbacks. These are required by the RZ/G2L SCIFA driver.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> >> ---
> >>
> >> Changes in v6:
> >> - set CHCTRL_SETSUS for pause and CHCTRL_CLRSUS for resume
> >> - dropped read-modify-update approach for CHCTRL updates as the
> >> HW returns zero when reading CHCTRL
> >> - moved the read_poll_timeout_atomic() under spin lock to
> >> ensure avoid any races b/w pause and resume functionalities
> >>
> >> Changes in v5:
> >> - used suspend capability of the controller to pause/resume
> >> the transfers
> >>
> >> drivers/dma/sh/rz-dmac.c | 36 ++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 36 insertions(+)
> >>
> >> diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c
> >> index 44f0f72cbcf1..377bdd5c9425
> >> 100644
> >> --- a/drivers/dma/sh/rz-dmac.c
> >> +++ b/drivers/dma/sh/rz-dmac.c
> >> @@ -135,10 +135,12 @@ struct rz_dmac {
> >> #define CHANNEL_8_15_COMMON_BASE 0x0700
> >>
> >> #define CHSTAT_ER BIT(4)
> >> +#define CHSTAT_SUS BIT(3)
> >> #define CHSTAT_EN BIT(0)
> >>
> >> #define CHCTRL_CLRINTMSK BIT(17)
> >> #define CHCTRL_CLRSUS BIT(9)
> >> +#define CHCTRL_SETSUS BIT(8)
> >> #define CHCTRL_CLRTC BIT(6)
> >> #define CHCTRL_CLREND BIT(5)
> >> #define CHCTRL_CLRRQ BIT(4)
> >> @@ -827,6 +829,38 @@ static enum dma_status rz_dmac_tx_status(struct dma_chan *chan,
> >> return status;
> >> }
> >>
> >> +static int rz_dmac_device_pause(struct dma_chan *chan) {
> >> + struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> >> + u32 val;
> >> + int ret;
> >> +
> >> + scoped_guard(spinlock_irqsave, &channel->vc.lock) {
> >
> >> + rz_dmac_ch_writel(channel, CHCTRL_SETSUS, CHCTRL, 1);
> >
> >
> > Probably first you need to check CHSTAT_EN first before setting CHCTRL_SETSUS??
> >
> > As per the hardware manual
> >
> > "
> > Suspends the current DMA transfer. Setting this bit to 1 when 1 is set
> > in EN of the CHSTAT_n/nS register can suspend the current DMA transfer."
>
> OK, I'll update it as follows:
>
> static int rz_dmac_device_pause(struct dma_chan *chan) {
> struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> u32 val;
>
> guard(spinlock_irqsave)(&channel->vc.lock);
>
> val = rz_dmac_ch_readl(channel, CHSTAT, 1);
> if (!(val & CHSTAT_EN))
> return 0;
>
> rz_dmac_ch_writel(channel, CHCTRL_SETSUS, CHCTRL, 1);
> return read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> (val & CHSTAT_SUS), 1, 1024,
> false, channel, CHSTAT, 1);
> }
OK.
>
> This avoids timeouts reported by read_poll_timeout_atomic() when pause is set for a disabled channel.
>
> >
> >
> >> + ret = read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> >> + (val & CHSTAT_SUS), 1, 1024,
> >> + false, channel, CHSTAT, 1);
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int rz_dmac_device_resume(struct dma_chan *chan) {
> >> + struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> >> + u32 val;
> >> + int ret;
> >> +
> >> + scoped_guard(spinlock_irqsave, &channel->vc.lock) {
> >
> >
> >> + rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1);
> >
> >
> > Similarly, first you need to check CHSTAT_SUS bit first and then clear suspend state.
> >
> >
> > Clears the suspend status. Setting this bit to 1 when 1 is set in SUS
> > of the CHSTAT_n/nS register can clear the suspend status.
>
> I'll update this one as follows, to keep the code simple:
>
> static int rz_dmac_device_resume(struct dma_chan *chan) {
> struct rz_dmac_chan *channel = to_rz_dmac_chan(chan);
> u32 val;
>
> guard(spinlock_irqsave)(&channel->vc.lock);
>
> /* Do not check CHSTAT_SUS but rely on HW capabilities. */
> rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1);
> return read_poll_timeout_atomic(rz_dmac_ch_readl, val,
> !(val & CHSTAT_SUS), 1, 1024,
> false, channel, CHSTAT, 1);
> }
>
> With this:
>
> 1/ in case the channel is not suspended and the CHCTRL_CLRSUS is set, the read_poll_timeout_atomic()
> will not timeout, as the default value of the CHSTAT_SUS is zero.
Just a question as we are not following the hardware manual.
At hardware level does it have any implications?
Eg: we set this write only register without the device being suspended
The next suspend operation, immediately clears the suspend operation
Or
does it work normally.
Cheers,
Biju
Powered by blists - more mailing lists