linux-kernel - Re: [PATCH 1/2] ASOC: rochchip: Simplify the condition logic in rockchip_sai_xfer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5104592.0VBMTVartN@workhorse>
Date: Wed, 04 Jun 2025 19:23:23 +0200
From: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
To: linux-rockchip@...ts.infradead.org, linux-sound@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Pei Xiao <xiaopei01@...inos.cn>
Cc: Pei Xiao <xiaopei01@...inos.cn>
Subject:
 Re: [PATCH 1/2] ASOC: rochchip: Simplify the condition logic in
 rockchip_sai_xfer_stop

On Wednesday, 4 June 2025 05:13:29 Central European Summer Time Pei Xiao wrote:
> cocci warning:
> ./sound/soc/rockchip/rockchip_sai.c:387:8-10:
> 	WARNING: possible condition with no effect (if == else)
> 
> Simplify the condition logic in rockchip_sai_xfer_stop() by removing the
> redundant SNDRV_PCM_STREAM_PLAYBACK branch. The modified logic now:
> 1. For stream < 0: handles both playback and capture
> 2. For all other cases (both PLAYBACK and CAPTURE):
>    sets playback = true and capture = false
> 
> Signed-off-by: Pei Xiao <xiaopei01@...inos.cn>
> ---
>  sound/soc/rockchip/rockchip_sai.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/sound/soc/rockchip/rockchip_sai.c b/sound/soc/rockchip/rockchip_sai.c
> index 602f1ddfad00..79b04770da1c 100644
> --- a/sound/soc/rockchip/rockchip_sai.c
> +++ b/sound/soc/rockchip/rockchip_sai.c
> @@ -384,9 +384,6 @@ static void rockchip_sai_xfer_stop(struct rk_sai_dev *sai, int stream)
>  	if (stream < 0) {
>  		playback = true;
>  		capture = true;
> -	} else if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
> -		playback = true;
> -		capture = false;
>  	} else {
>  		playback = true;
>  		capture = false;
> 

You can probably get rid of the locals playback and capture altogether:

    static void rockchip_sai_xfer_stop(struct rk_sai_dev *sai, int stream)
    {
        unsigned int msk, val, clr;

        msk = SAI_XFER_TXS_MASK;
        val = SAI_XFER_TXS_DIS;
        clr = SAI_CLR_TXC;
        
        if (stream < 0) {
            msk |= SAI_XFER_RXS_MASK;
            val |= SAI_XFER_RXS_DIS;
            clr |= SAI_CLR_RXC;
        }

        regmap_update_bits(sai->regmap, SAI_XFER, msk, val);
        rockchip_sai_poll_stream_idle(sai, true, stream < 0);

        rockchip_sai_clear(sai, clr);
    }

but this in general makes me suspicious of the intent of the code in
the first place. Playback always being true and capture only being
true if playback is also true seems odd. Checking the callsites of
this function confirms my suspicions that while this fixes the cocci
warning, it just means the code is now intentionally broken.

This here may be closer to the original intent:

    static void rockchip_sai_xfer_stop(struct rk_sai_dev *sai, int stream)
    {
            unsigned int msk = 0, val = 0, clr = 0;
            bool capture = stream == SNDRV_PCM_STREAM_CAPTURE || stream < 0;
            /* could be <= 0 but we don't want to depend on enum values */
            bool playback = stream == SNDRV_PCM_STREAM_PLAYBACK || stream < 0;

            if (playback) {
                    msk |= SAI_XFER_TXS_MASK;
                    val |= SAI_XFER_TXS_DIS;
                    clr |= SAI_CLR_TXC;
            }

            if (capture) {
                    msk |= SAI_XFER_RXS_MASK;
                    val |= SAI_XFER_RXS_DIS;
                    clr |= SAI_CLR_RXC;
            }

            regmap_update_bits(sai->regmap, SAI_XFER, msk, val);
            rockchip_sai_poll_stream_idle(sai, playback, capture);

            rockchip_sai_clear(sai, clr);
    }


Please let me know whether this looks right to you.

Kind regards,
Nicolas Frattaroli