On Mon, Mar 25, 2024 at 11:40:33AM +0800, Su Hui wrote:
> Clang static checker(scan-build):
> sound/soc/sti/uniperif_player.c:1115:12: warning:
> The result of the left shift is undefined because the right operand is
> negative [core.UndefinedBinaryOperatorResult]
>
> When UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) equals to -1, the result of
> SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) is undefined.
>
> Here are some results of using different compilers.
> 1UL >> -1 1UL << -1
> gcc 10.2.1 0x2 0
> gcc 11.4.0 0 0x8000000000000000
> clang 14.0.0 0x64b8a45d72a0 0x64b8a45d72a0
> clang 17.0.0 0x556c43b0f2a0 0x556c43b0f2a0
>
> Add some macros to ensure that when right opreand is negative or other
> invalid values, the results of bitwise shift is zero.
>
> Fixes: e1ecace6a685 ("ASoC: sti: Add uniperipheral header file")
> Signed-off-by: Su Hui <suhui@xxxxxxxxxxxx>
> ---
> sound/soc/sti/uniperif.h | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/sti/uniperif.h b/sound/soc/sti/uniperif.h
> index 2a5de328501c..1cbff01fbff0 100644
> --- a/sound/soc/sti/uniperif.h
> +++ b/sound/soc/sti/uniperif.h
> @@ -12,17 +12,28 @@
>
> #include <sound/dmaengine_pcm.h>
>
> +#define SR_SHIFT(a, b) ({unsigned long __a = (a); \
> + unsigned int __b = (b); \
> + __b < BITS_PER_LONG ? \
> + __a >> __b : 0; })
The code definitely looks buggy, but how do you know your solution is
correct without testing it?
I don't like this solution at all. This is basically a really
complicated way of writing "if (b != -1)". Instead of checking for -1,
the better solution is to just stop passing -1. If you review that
file, every time it uses "-1" that's either dead code or a bug...
sound/soc/sti/uniperif.h
132 #define UNIPERIF_ITS_UNDERFLOW_REC_DONE_SHIFT(ip) \
133 ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? -1 : 12)
^^^^
This is dead code
134 #define UNIPERIF_ITS_UNDERFLOW_REC_DONE_MASK(ip) \
135 ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? \
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
because the version is checked here.
136 0 : (BIT(UNIPERIF_ITS_UNDERFLOW_REC_DONE_SHIFT(ip))))
Just delete UNIPERIF_ITS_UNDERFLOW_REC_DONE_SHIFT() and use 12 directly.
[ snip ]
988 #define UNIPERIF_BIT_CONTROL_OFFSET(ip) \
989 ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? -1 : 0x004c)
^^^
Negative offsets seem like a bug.
990 #define GET_UNIPERIF_BIT_CONTROL(ip) \
991 readl_relaxed(ip->base + UNIPERIF_BIT_CONTROL_OFFSET(ip))
regards,
dan carpenter
