From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 93C71375ACF for ; Mon, 20 Apr 2026 12:52:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.221.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776689547; cv=none; b=YgWGjAoH1E86mu/rTZ0CIZT3apcu3p/ZGeCVfjK7SzitAvkdQjMwjM/1Lp1eww/iEdLsmTghulrIdiKrdID1fp1xBiF5Thue8ONWSgke/y8MdFM0jXj5VZYphzNwg+WvA/IAIYCC90mPbuFyFGM2Y9RGuvCWbLKkcmk/YgbT5Lo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776689547; c=relaxed/simple; bh=wE/nLJYcfwfiHpxeFEQVE8m77fQJ78pDtEdAEnuEGZ0=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=WDdxjs1pwJfRgUAbYYyRcXTCVqLDvYB1MsoICXuoNgZgE0gakZCp6QgwqZ+r4PI9fR6TPqzM/1R6paELaMMsYAa2sYYAC6G16mZ0xTPNjQSVpX8EtG+79pH7EhPBGlhHrha/5fcvM614JH4BUkHkSyQbztj0O9ArAikGHGzw2iw= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev; spf=pass smtp.mailfrom=tuxon.dev; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b=c7EUpqbQ; arc=none smtp.client-ip=209.85.221.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b="c7EUpqbQ" Received: by mail-wr1-f44.google.com with SMTP id ffacd0b85a97d-43cfbd17589so2294797f8f.0 for ; Mon, 20 Apr 2026 05:52:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1776689544; x=1777294344; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=jyZEuicI8WQZweXVv7pRfSwXG7shncGVxuqHWPlSo+U=; b=c7EUpqbQ7kX6u+QhYsOte7xn26QbC+VdjvFYN/lcZO8C7EAlciz87rUG+mvzqFBO4N zECgCo2smOPX8Nt3M9ThcxLQ77f/UNcn84cWjDo8zgino7Y58dTI9O9nWe263Ki8ujz1 FDEAsF1C+ZiwL/6ArM7F/zcKqDSj9u0QEw7lebFArUN9KYMdV/vbagDLHLLjnptewfzY Tu7xCp7wFIJJenZ2ughimvuQ/Ph8cuV7fktVI++cEdd8GQ8kLXvWVjE2LEtg3h7XVyXM mt0zlu/3dMxcKp3Tk8c48FxZ0/kTJ9WPjM6jwIdMCCLzSrB+4nhdQvHVCf/6i9nvsFTh Zbtg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776689544; x=1777294344; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jyZEuicI8WQZweXVv7pRfSwXG7shncGVxuqHWPlSo+U=; b=ijgYgQg9zolD6Ac4F2ocjIzGsy4QLWOz0LmmugZUGb0NAYW5nQSZ49sgcPE/AFDKOK Rks/VeykeVIounJl1NipYD72dx8fcudiz3jtsaoVFQf/dTzkv73z5xu9us1vIIaUHUPs yNBDP/OsdPfYvoMVA+zDkJJh8FocThzWj8aIX7RCN4vMsTVJsjuo3dz5/M9p/EWKk2XM hi0/Lhgb7fjspX/CvjpVFW/ZgDkun8W3YGxqKx++q56+zPcGsemmO9L9AxgowRksIAky pQmAg7wOzh6hFO45lEPUAPT9BRNsd94YswYVMwL4nOrv6StJwNQ8MZ5nwxkGgru4m2uh vUNQ== X-Forwarded-Encrypted: i=1; AFNElJ+0JzRJ36yCARQN6g0dsO+4hoIxM9pbObwOdlbRRrRVo/MnKlWyqzAXTUh72A11ycShj3LiV0Navmf9nXQ=@vger.kernel.org X-Gm-Message-State: AOJu0YwNZpv8SREabZdVf5SGYrOPtIQ+wKFgY+sunWuyv0CzFAD+JTcY bSnfVjMorHSqHEqzK5UZOr2LzyYWmIITwydkvPH14jv8MGx0BretI9+iCEHCwnwvaHk= X-Gm-Gg: AeBDiesPr3anzVJuD7AX5Xb7ibFTKlEnVDt50byw8xhTtlsO64XtnYJbvWw5FMjP1dV PFhmWCQ8cKLyp9cbAmGry2MghFNgsj9JofI2oYZ1E/gAbozTfBqMFmtHI4JSIkj9twmNU9rkt3+ SeeA3Db8jNwcWNcvwpUl6dmw2/rWscJtEeHnsxgxLzX2Iux3gnZ4y2kTm3tY4uIZ7f3ZtHVAxXg KIRa/GLSibOu1uU25yb1J7y3FyON6rnII++m6xNQL+l8Ixy+lpMs7JRO4Y2XVRfuIOI5b6YUTBb lVsrXpX9HpoqpR6Z8+wcllLtnPCzWoAlIi6JeaaAItSGKBSEOO0K3k7GpKlqzTNnNNK/G849gUg pHwjBlxwcnpyyPPFeexDzlvmwYTNJpGI6jH2JuzdXYGXEm1/PZ3I7WZksDfvXwojZGoVqxrK6Vg oRBjWbQB9ZcS2nARewHoR8/A4QXZ67CdAwJQZAW66y0g== X-Received: by 2002:a5d:4535:0:b0:43f:e22f:606 with SMTP id ffacd0b85a97d-43fe3da6680mr13443062f8f.2.1776689543971; Mon, 20 Apr 2026 05:52:23 -0700 (PDT) Received: from [192.168.50.4] ([82.78.167.123]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-43fe4e44f69sm30902955f8f.25.2026.04.20.05.52.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Apr 2026 05:52:23 -0700 (PDT) Message-ID: Date: Mon, 20 Apr 2026 15:52:22 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: sashiko.dev review (Re: [PATCH v4 14/17] dmaengine: sh: rz-dmac: Add suspend to RAM support) To: vkoul@kernel.org, Frank.Li@kernel.org, lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz, tiwai@suse.com, biju.das.jz@bp.renesas.com, prabhakar.mahadev-lad.rj@bp.renesas.com, p.zabel@pengutronix.de, geert+renesas@glider.be, fabrizio.castro.jz@renesas.com, long.luu.ur@renesas.com Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sound@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Claudiu Beznea References: <20260411114303.2814115-1-claudiu.beznea.uj@bp.renesas.com> <20260411114303.2814115-15-claudiu.beznea.uj@bp.renesas.com> Content-Language: en-US From: Claudiu Beznea In-Reply-To: <20260411114303.2814115-15-claudiu.beznea.uj@bp.renesas.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit > commit e7af08b0f2dc569cc63b17e07dd4e506e50fee28 > Author: Claudiu Beznea > dmaengine: sh: rz-dmac: Add suspend to RAM support > The Renesas RZ/G3S SoC supports a power saving mode in which power to most > of the SoC components is turned off, including the DMA IP. Add suspend to > RAM support to save and restore the DMA IP registers. > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c > > --- a/drivers/dma/sh/rz-dmac.c > > +++ b/drivers/dma/sh/rz-dmac.c > [ ... ] > > @@ -1354,6 +1407,130 @@ static void rz_dmac_remove(struct platform_device *pdev) > > pm_runtime_disable(&pdev->dev); > > } > > > > +static int rz_dmac_suspend_prepare(struct device *dev) > > +{ > > + struct rz_dmac *dmac = dev_get_drvdata(dev); > > + > > + for (unsigned int i = 0; i < dmac->n_channels; i++) { > > + struct rz_dmac_chan *channel = &dmac->channels[i]; > > + > > + guard(spinlock_irqsave)(&channel->vc.lock); > > + > > + /* Wait for transfer completion, except in cyclic case. */ > > + if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) > > + continue; > > + > > + if (rz_dmac_chan_is_enabled(channel)) > > + return -EAGAIN; > Is it intended that this aborts the system suspend instead of waiting? Returning -EAGAIN here makes the core suspend code to keep trying calling the prepare API of this driver. That's why it has mentioned waiting. > The commit message states the .prepare callback "waits for all the ongoing > transfers to complete before allowing suspend-to-RAM to proceed." > However, returning -EAGAIN from a system sleep .prepare callback immediately > aborts the system suspend process instead of waiting. > Additionally, since the PM core executes the .prepare phase for all devices > before executing any .suspend callbacks, consumer devices haven't yet run > their own .suspend callbacks to stop their DMA transfers. > If any normal background DMA activity is ongoing, won't this immediately > abort the entire system suspend transition? If DMA activity is ongoing the expectation is that it will finish and the suspend_prepare() will go forward. > > + } > > + > > + return 0; > > +} > [ ... ] > > +static int rz_dmac_suspend(struct device *dev) > > +{ > > + struct rz_dmac *dmac = dev_get_drvdata(dev); > > + int ret; > Can ret be used uninitialized here? Yes, this is a typo. I'll properly initialize this here. > If the DMA controller has no cyclic channels configured, the loop below will > skip every iteration due to the continue statement, leaving ret > uninitialized. > > + > > + for (unsigned int i = 0; i < dmac->n_channels; i++) { > > + struct rz_dmac_chan *channel = &dmac->channels[i]; > > + > > + guard(spinlock_irqsave)(&channel->vc.lock); > > + > > + if (!(channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC))) > > + continue; > > + > > + ret = rz_dmac_device_pause_internal(channel); > > + if (ret) { > > + dev_err(dev, "Failed to suspend channel %s\n", > > + dma_chan_name(&channel->vc.chan)); > > + break; > > + } > > + > > + channel->pm_state.nxla = rz_dmac_ch_readl(channel, NXLA, 1); > > + } > > + > > + if (ret) { > If ret contains uninitialized stack memory that evaluates to true, could this > trigger the error recovery path and silently abort the system suspend > transition? > > + rz_dmac_suspend_recover(dmac); > > + return ret; > > + } > > + > > + pm_runtime_put_sync(dmac->dev); > > + > > + ret = reset_control_assert(dmac->rstc); > Is the ordering of runtime PM and reset control inverted here compared to > rz_dmac_remove()? Yes, I'll adjust it. > In rz_dmac_remove(), reset_control_assert() is called before pm_runtime_put(). > Operating on the reset control after the device's power domain is suspended > might cause the operations to be silently ignored by the hardware or lead to > system bus lockups. > > + if (ret) { > > + pm_runtime_resume_and_get(dmac->dev); > > + rz_dmac_suspend_recover(dmac); > > + } > > + > > + return ret; > > +} > > + > > +static int rz_dmac_resume(struct device *dev) > > +{ > > + struct rz_dmac *dmac = dev_get_drvdata(dev); > > + int errors = 0, ret; > > + > > + ret = reset_control_deassert(dmac->rstc); > Similarly, is the ordering of reset control and runtime PM inverted here > compared to rz_dmac_probe()? Yes, I'll adjust it. > In rz_dmac_probe(), pm_runtime_resume_and_get() is called before > reset_control_deassert(). Doing this in the reverse order here means the > reset is deasserted before the hardware power domain is enabled. > > + if (ret) > > + return ret; > > + > > + ret = pm_runtime_resume_and_get(dmac->dev); > > + if (ret) { > > + reset_control_assert(dmac->rstc); > > + return ret; > > + } > [ ... ] > > @@ -1380,6 +1557,7 @@ static struct platform_driver rz_dmac_driver = { > > .driver = { > > .name = "rz-dmac", > > .of_match_table = of_rz_dmac_match, > > + .pm = pm_sleep_ptr(&rz_dmac_pm_ops), > Should this use pm_ptr() instead of pm_sleep_ptr()? > If CONFIG_PM_SLEEP is disabled, pm_sleep_ptr() evaluates to NULL, but the > struct rz_dmac_pm_ops itself and its unconditional callbacks like > rz_dmac_suspend_prepare will remain fully compiled into the binary as unused > dead code, potentially leading to compiler warnings. > Using pm_ptr() for the dev_pm_ops structure pointer and pm_sleep_ptr() > around the sleep callbacks inside the structure might resolve this. > > }, > > .probe = rz_dmac_probe, > > .remove = rz_dmac_remove, I think pm_sleep_ptr() fits better here as CONFIG_PM_SLEEP depends on CONFIG_SUSPEND which is the flag under which the system suspend/resume code resides. Also, CONFIG_PM_SLEEP selects CONFIG_PM, so CONFIG_PM_SLEEP cannot be enabled w/o CONFIG_PM.