Messages in this thread | | | Date | Wed, 27 Mar 2024 09:15:18 +0800 | Subject | Re: [PATCH] ASoC: sti: uniperif: fix the undefined bitwise shift behavior problem | From | 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 > >
| |