| Message ID | 20240325034032.1031885-1-suhui@nfschina.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sti: uniperif: fix the undefined bitwise shift behavior problem | expand |
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@nfschina.com> > --- > 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
Hi, On 2024/3/25 22:25, Dan Carpenter wrote: > On Mon, Mar 25, 2024 at 11:40:33AM +0800, Su Hui wrote: >> --- 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 only test some cases like SR_SHIFT(1, -1),SR_SHIFT(8,1), it seems have a right result. Oh, maybe I understand it. When 'a' is a negative value like '(int)-1', SR_SHIFT(a, b) will have some bugs. > > 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... Agreed，some are dead codes which can be removed, but what should we do with the following error codes like this one? sound/soc/sti/uniperif.h 415 #define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \ 416 ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1) ... 423 #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \ 424 SET_UNIPERIF_REG(ip, \ 425 UNIPERIF_CONFIG_OFFSET(ip), \ 426 UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ How about this solution? If the condition is false, just skip it. @@ -412,8 +412,7 @@ UNIPERIF_CONFIG_REPEAT_CHL_STS_MASK(ip), 1) /* BACK_STALL_REQ */ -#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \ - ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1) +#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) 7 #define UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip) 0x1 #define GET_UNIPERIF_CONFIG_BACK_STALL_REQ(ip) \ GET_UNIPERIF_REG(ip, \ @@ -421,10 +420,11 @@ UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \ UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip)) #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \ + ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? -1 : \ SET_UNIPERIF_REG(ip, \ UNIPERIF_CONFIG_OFFSET(ip), \ UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \ - UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0) + UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0)) #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_ENABLE(ip) \ Maybe should print some error log here. I'm not sure about the safety of skipping SET_UNIPERIF_REG when the condition is false, Would it be better to make the result of undefined shift equal to zero? regards, Su Hui
This is a kindly resend email. Sorry for the error style of last email :( On 2024/3/26 13:30, Su Hui wrote: > Hi, > On 2024/3/25 22:25, Dan Carpenter wrote: >> On Mon, Mar 25, 2024 at 11:40:33AM +0800, Su Hui wrote: >>> --- 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 only test some cases like SR_SHIFT(1, -1),SR_SHIFT(8,1), it seems have a right result. > Oh, maybe I understand it. When 'a' is a negative value like '(int)-1', SR_SHIFT(a, b) will > have some bugs. >> 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... > Agreed，some are dead codes which can be removed, but what should we do with the > following error codes like this one? > sound/soc/sti/uniperif.h > 415 #define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \ > 416 ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1) > ... > 423 #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \ > 424 SET_UNIPERIF_REG(ip, \ > 425 UNIPERIF_CONFIG_OFFSET(ip), \ > 426 UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \ > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > How about this solution? If the condition is false, just skip it. > > @@ -412,8 +412,7 @@ > UNIPERIF_CONFIG_REPEAT_CHL_STS_MASK(ip), 1) > > /* BACK_STALL_REQ */ > -#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) \ > - ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? 7 : -1) > +#define UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip) 7 > #define UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip) 0x1 > #define GET_UNIPERIF_CONFIG_BACK_STALL_REQ(ip) \ > GET_UNIPERIF_REG(ip, \ > @@ -421,10 +420,11 @@ > UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \ > UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip)) > #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_DISABLE(ip) \ > + ((ip)->ver < SND_ST_UNIPERIF_VERSION_UNI_PLR_TOP_1_0 ? -1 : \ > SET_UNIPERIF_REG(ip, \ > UNIPERIF_CONFIG_OFFSET(ip), \ > UNIPERIF_CONFIG_BACK_STALL_REQ_SHIFT(ip), \ > - UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0) > + UNIPERIF_CONFIG_BACK_STALL_REQ_MASK(ip), 0)) > #define SET_UNIPERIF_CONFIG_BACK_STALL_REQ_ENABLE(ip) \ > > Maybe should print some error log here. > I'm not sure about the safety of skipping SET_UNIPERIF_REG when the condition is false, > > Would it be better to make the result of undefined shift equal to zero? > > regards, > Su Hui > >
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; }) + +#define SL_SHIFT(a, b) ({unsigned long __a = (a); \ + unsigned int __b = (b); \ + __b < BITS_PER_LONG ? \ + __a << __b : 0; }) + /* * Register access macros */ #define GET_UNIPERIF_REG(ip, offset, shift, mask) \ - ((readl_relaxed(ip->base + offset) >> shift) & mask) + (SR_SHIFT(readl_relaxed(ip->base + offset), shift) & mask) #define SET_UNIPERIF_REG(ip, offset, shift, mask, value) \ writel_relaxed(((readl_relaxed(ip->base + offset) & \ - ~(mask << shift)) | (((value) & mask) << shift)), ip->base + offset) + ~SL_SHIFT(mask, shift)) | SL_SHIFT(((value) & mask), shift)),\ + ip->base + offset) #define SET_UNIPERIF_BIT_REG(ip, offset, shift, mask, value) \ - writel_relaxed((((value) & mask) << shift), ip->base + offset) + writel_relaxed(SL_SHIFT(((value) & mask), shift), ip->base + offset) /* * UNIPERIF_SOFT_RST reg
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@nfschina.com> --- sound/soc/sti/uniperif.h | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)