| Message ID | 20181218081657.GA32567@kadam (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: fix a ! vs ~ typo | expand |
Hi Dan On 12/18/18 9:16 AM, Dan Carpenter wrote: > This is from static analysis and not from testing. It doesn't really > make sense to talk about !BIT(0) so probably ~BIT(0) was intended. > I had to cast it from unsigned long to unsigned int to silence a GCC > warning. > > Fixes: bb6869b21478 ("Peter Griffin <peter.griffin@linaro.org>") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > > Sorry for the crap commit message. I'm pretty sure the static checker > warning is correct, but I don't know what the run time impact of this > bug would be. > > drivers/remoteproc/st_slim_rproc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/st_slim_rproc.c b/drivers/remoteproc/st_slim_rproc.c > index d711d9430a4f..a646dfb9d6cd 100644 > --- a/drivers/remoteproc/st_slim_rproc.c > +++ b/drivers/remoteproc/st_slim_rproc.c > @@ -128,7 +128,7 @@ static int slim_rproc_start(struct rproc *rproc) > writel(SLIM_STBUS_SYNC_DIS, slim_rproc->peri + SLIM_STBUS_SYNC_OFST); > > /* enable cpu pipeline clock */ > - writel(!SLIM_CLK_GATE_DIS, > + writel((unsigned int)~SLIM_CLK_GATE_DIS, > slim_rproc->slimcore + SLIM_CLK_GATE_OFST); > > /* clear int & cmd mailbox */ > @@ -167,7 +167,7 @@ static int slim_rproc_stop(struct rproc *rproc) > /* disable cpu pipeline clock */ > writel(SLIM_CLK_GATE_DIS, slim_rproc->slimcore + SLIM_CLK_GATE_OFST); > > - writel(!SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST); > + writel((unsigned int)~SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST); > > val = readl(slim_rproc->slimcore + SLIM_EN_OFST); > if (val & SLIM_EN_RUN) > Writing !SLIM_CLK_GATE_DIS or (unsigned int)~SLIM_CLK_GATE_DIS don't give the same result: !SLIM_CLK_GATE_DIS = 0 (unsigned int)~SLIM_CLK_GATE_DIS = 0xfffffffe Patrice
On Thu, Dec 20, 2018 at 10:05:55AM +0000, Patrice CHOTARD wrote: > > - writel(!SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST); > > + writel((unsigned int)~SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST); > > > > val = readl(slim_rproc->slimcore + SLIM_EN_OFST); > > if (val & SLIM_EN_RUN) > > > > Writing !SLIM_CLK_GATE_DIS or (unsigned int)~SLIM_CLK_GATE_DIS don't > give the same result: > > !SLIM_CLK_GATE_DIS = 0 > > (unsigned int)~SLIM_CLK_GATE_DIS = 0xfffffffe Right. !BIT(0) is zero. I'm pretty sure we meant to do a bitwise negate and not a logical negate from looking at the code, but I don't know the real world impact and can't test it. It's kind of a crap patch, but I think it's correct. regards, dan carpenter
On Thu, Dec 20, 2018 at 10:05:55AM +0000, Patrice CHOTARD wrote: > Hi Dan > > On 12/18/18 9:16 AM, Dan Carpenter wrote: > > This is from static analysis and not from testing. It doesn't really > > make sense to talk about !BIT(0) so probably ~BIT(0) was intended. > > I had to cast it from unsigned long to unsigned int to silence a GCC > > warning. > > > > Fixes: bb6869b21478 ("Peter Griffin <peter.griffin@linaro.org>") And oh... I put Peter's email here instead of the commit title. Sorry. I will resend if we decide the patch is required. regards, dan carpenter
diff --git a/drivers/remoteproc/st_slim_rproc.c b/drivers/remoteproc/st_slim_rproc.c index d711d9430a4f..a646dfb9d6cd 100644 --- a/drivers/remoteproc/st_slim_rproc.c +++ b/drivers/remoteproc/st_slim_rproc.c @@ -128,7 +128,7 @@ static int slim_rproc_start(struct rproc *rproc) writel(SLIM_STBUS_SYNC_DIS, slim_rproc->peri + SLIM_STBUS_SYNC_OFST); /* enable cpu pipeline clock */ - writel(!SLIM_CLK_GATE_DIS, + writel((unsigned int)~SLIM_CLK_GATE_DIS, slim_rproc->slimcore + SLIM_CLK_GATE_OFST); /* clear int & cmd mailbox */ @@ -167,7 +167,7 @@ static int slim_rproc_stop(struct rproc *rproc) /* disable cpu pipeline clock */ writel(SLIM_CLK_GATE_DIS, slim_rproc->slimcore + SLIM_CLK_GATE_OFST); - writel(!SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST); + writel((unsigned int)~SLIM_EN_RUN, slim_rproc->slimcore + SLIM_EN_OFST); val = readl(slim_rproc->slimcore + SLIM_EN_OFST); if (val & SLIM_EN_RUN)
This is from static analysis and not from testing. It doesn't really make sense to talk about !BIT(0) so probably ~BIT(0) was intended. I had to cast it from unsigned long to unsigned int to silence a GCC warning. Fixes: bb6869b21478 ("Peter Griffin <peter.griffin@linaro.org>") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- Sorry for the crap commit message. I'm pretty sure the static checker warning is correct, but I don't know what the run time impact of this bug would be. drivers/remoteproc/st_slim_rproc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)