[PATCH] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning

[PATCH] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning

[Jay Fang]

drivers/spi/spi-cadence-quadspi.c:267:18: warning: Shifting signed 32-bit
value by 31 bits is undefined behaviour [shiftTooManyBitsSigned]
    return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
                    ^

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jay Fang <f.fangjian@huawei.com>
---
 drivers/spi/spi-cadence-quadspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 442cc7c..9a2798a5 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -264,7 +264,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
 {
 	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
 
-	return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
+	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);
 }
 
 static u32 cqspi_get_rd_sram_level(struct cqspi_st *cqspi)
-- 
2.7.4

Re: [PATCH] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning

[Dan Carpenter]

On Thu, Mar 04, 2021 at 06:47:52PM +0800, Jay Fang wrote:
> drivers/spi/spi-cadence-quadspi.c:267:18: warning: Shifting signed 32-bit
> value by 31 bits is undefined behaviour [shiftTooManyBitsSigned]
>     return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
>                     ^
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jay Fang <f.fangjian@huawei.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 442cc7c..9a2798a5 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -264,7 +264,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
>  {
>  	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
>  
> -	return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
> +	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);

This is always going to be false because reg is a u32.

regards,
dan carpenter

Re: [PATCH] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning

[Pratyush Yadav]

On 04/03/21 04:08PM, Dan Carpenter wrote:
> On Thu, Mar 04, 2021 at 06:47:52PM +0800, Jay Fang wrote:
> > drivers/spi/spi-cadence-quadspi.c:267:18: warning: Shifting signed 32-bit
> > value by 31 bits is undefined behaviour [shiftTooManyBitsSigned]
> >     return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
> >                     ^
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Jay Fang <f.fangjian@huawei.com>
> > ---
> >  drivers/spi/spi-cadence-quadspi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > index 442cc7c..9a2798a5 100644
> > --- a/drivers/spi/spi-cadence-quadspi.c
> > +++ b/drivers/spi/spi-cadence-quadspi.c
> > @@ -264,7 +264,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
> >  {
> >  	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> >  
> > -	return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
> > +	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);
> 
> This is always going to be false because reg is a u32.

Hmm... I don't see why it would always be false. reg would promoted to 
unsigned long and the result should then depend on the actual value of 
the bit, which can be represented by an unsigned long. There is no loss 
of information.

Anyway, it still makes more sense to make it 1U because reg is u32. Just 
keep the types same and avoid all the conversion rules.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning

[Dan Carpenter]

On Fri, Mar 05, 2021 at 01:04:27AM +0530, Pratyush Yadav wrote:
> On 04/03/21 04:08PM, Dan Carpenter wrote:
> > On Thu, Mar 04, 2021 at 06:47:52PM +0800, Jay Fang wrote:
> > > drivers/spi/spi-cadence-quadspi.c:267:18: warning: Shifting signed 32-bit
> > > value by 31 bits is undefined behaviour [shiftTooManyBitsSigned]
> > >     return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
> > >                     ^
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Jay Fang <f.fangjian@huawei.com>
> > > ---
> > >  drivers/spi/spi-cadence-quadspi.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > > index 442cc7c..9a2798a5 100644
> > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > @@ -264,7 +264,7 @@ static bool cqspi_is_idle(struct cqspi_st *cqspi)
> > >  {
> > >  	u32 reg = readl(cqspi->iobase + CQSPI_REG_CONFIG);
> > >  
> > > -	return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
> > > +	return reg & (1UL << CQSPI_REG_CONFIG_IDLE_LSB);
> > 
> > This is always going to be false because reg is a u32.
> 
> Hmm... I don't see why it would always be false. reg would promoted to 
> unsigned long and the result should then depend on the actual value of 
> the bit, which can be represented by an unsigned long. There is no loss 
> of information.
> 
> Anyway, it still makes more sense to make it 1U because reg is u32. Just 
> keep the types same and avoid all the conversion rules.

Ah, crap.  I'm sorry.  I somehow thought when I forwarded this bug the
other day that CQSPI_REG_CONFIG_IDLE_LSB was more than 31.

regards,
dan carpenter

Re: [PATCH] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning

[Mark Brown]

On Thu, 4 Mar 2021 18:47:52 +0800, Jay Fang wrote:
> drivers/spi/spi-cadence-quadspi.c:267:18: warning: Shifting signed 32-bit
> value by 31 bits is undefined behaviour [shiftTooManyBitsSigned]
>     return reg & (1 << CQSPI_REG_CONFIG_IDLE_LSB);
>                     ^

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: cadence-quadspi: Silence shiftTooManyBitsSigned warning
      commit: 55794b1d8623f73d9a4bf12e4343bc8fc96024e1

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark