From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 60BD233FE02 for ; Tue, 26 May 2026 09:28:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779787712; cv=none; b=aklSDAqw5gdWwQXpktY2CVD9OXn8ueviwyPl45J1DKXjF0Pkt+A6nJ0Srt+3qGZpKwNefYrc3qJPMoh+RCDYO3kjJNZ9z9Qjp+WMQZAtc9UGEvlOzFbHHUTooDPqnONzbki8Qmgv88ePkjLexPNFGXBel6MYMvuzkdMIP77OqMY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1779787712; c=relaxed/simple; bh=zYrhVd7TOC806dry2q67Exm5p44Q6vJeAi/2ZQDSO2Y=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=j8bwyiPXsE2yHHjGI74LC/bTzbD0Y1dEBS/80y8Ru3iNoQbcgQ6PAgF9STBOrGQOX50GTRtcAfULfcKDz6HwHjdvLZ89e8TsvR0g5/B/AHbAaK2NOtwmkb4MQFJQZD5zohkwpNahKzado3KujQPRzQEg9YhM4iYA6KVZVUMUnKs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=KcQ/5z4i; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="KcQ/5z4i" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E5F321F000E9; Tue, 26 May 2026 09:28:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1779787711; bh=2nQVUb7BmP1CsTwCyCiugUucUrJWlHy68zgQYD76JEc=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=KcQ/5z4ivENpqGbgWkksVhnM4oqbtUp66nGPX/stoyX8KBsscLt6rnirORh8j8MIS S7CD/cTxw/4a8LR2HpPLpBR+zgoDXUW7zsTrTknhuKZSq/GEdL59lP+F3JHf9fCwYN Qi3L17g+cII/H/9dLHC4I7Db/NHXwOXvZqF2Ah7lJoQXz5erx+PzzCuCpepN3pHK7k A4/CA/1wdn8TZjhkJWWcNOC3yV/Jn3KQDo1tlpAM1pa+S2xjSN5B8pZ4rqVIwwPl6m 6/+hnUGzjoqzOsk7Gr1DNK8YNm0QObzY/vKzzFTFDNiI5BpAnUWAVhwY5rQKCFqJFm m8kX30NLphdxg== From: sashiko-bot@kernel.org Subject: Re: [PATCH v6 10/18] dmaengine: sh: rz-dmac: Refactor pause/resume code Reply-To: sashiko-reviews@lists.linux.dev To: "Claudiu Beznea" Cc: dmaengine@vger.kernel.org, wsa+renesas@sang-engineering.com, Frank.Li@kernel.org, vkoul@kernel.org In-Reply-To: <20260526084710.3491480-11-claudiu.beznea@kernel.org> References: <20260526084710.3491480-11-claudiu.beznea@kernel.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Tue, 26 May 2026 09:28:30 +0000 Message-Id: <20260526092830.E5F321F000E9@smtp.kernel.org> Precedence: bulk X-Mailing-List: dmaengine@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 2 potential issue(= s) to consider: - [High] Software status tracking becomes inconsistent if hardware pause/re= sume operations time out. - [High] Unconditional hardware resume ignores other active pause requests. -- commit 9f2ce07d8501ca3cb4ac77194345402583e5ba4b Author: Claudiu Beznea dmaengine: sh: rz-dmac: Refactor pause/resume code This refactors the pause/resume support in the rz-dmac driver. It introduces a bitmask-based status field to allow reusing the pause and resume code for system suspend/resume and cyclic DMA operations. > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c > index 1f884ec101f80..557364443a5fd 100644 > --- a/drivers/dma/sh/rz-dmac.c > +++ b/drivers/dma/sh/rz-dmac.c [ ... ] > @@ -795,35 +811,70 @@ static enum dma_status rz_dmac_tx_status(struct dma= _chan *chan, [ ... ] > +static int rz_dmac_device_resume_set(struct rz_dmac_chan *channel, > + unsigned long clear_bitmask) > +{ > + int ret =3D 0; > + u32 val; > + > + lockdep_assert_held(&channel->vc.lock); > + > + /* Do not check CHSTAT_SUS but rely on HW capabilities. */ > + > + rz_dmac_ch_writel(channel, CHCTRL_CLRSUS, CHCTRL, 1); [Severity: High] Does this unconditional hardware resume ignore other active pause requests? The bitmask-based status tracking appears to be designed to handle multiple independent pause conditions. However, writing CHCTRL_CLRSUS happens regardless of the remaining state. If a channel is paused for multiple reasons, clearing one condition will immediately unpause the hardware while other constraints are still active. Could this regression lead to unexpected DMA execution and data corruption? > + ret =3D read_poll_timeout_atomic(rz_dmac_ch_readl, val, > + !(val & CHSTAT_SUS), 1, 1024, false, > + channel, CHSTAT, 1); > + > + channel->status &=3D ~clear_bitmask; [Severity: High] Should channel->status be cleared if the hardware resume operation times ou= t? If read_poll_timeout_atomic returns an error like -ETIMEDOUT, the status bitmask is still unconditionally cleared. If a caller later attempts to retry the resume operation, it will exit early because the status bit is already gone, leaving the hardware permanently suspended while the software stack thinks it is active. Does this create a regression where the channel can no longer be resumed? > + > + return ret; > +} --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260526084710.3491= 480-1-claudiu.beznea@kernel.org?part=3D10