Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
Hi Colin,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[cannot apply to v5.3-rc4 next-20190814]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Colin-King/scsi-mvumi-fix-32-bit-shift-of-a-u32-value/20190814-214025
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot
All warnings (new ones prefixed by >>):
drivers/scsi/mvumi.c: In function 'mvumi_delete_internal_cmd':
>> drivers/scsi/mvumi.c:299:38: warning: left shift count >= width of type
>> [-Wshift-count-overflow]
(dma_addr_t) m_sg->baseaddr_h << 32;
^~
vim +299 drivers/scsi/mvumi.c
285
286 static void mvumi_delete_internal_cmd(struct mvumi_hba *mhba,
287 struct mvumi_cmd *cmd)
288 {
289 struct mvumi_sgl *m_sg;
290 unsigned int size;
291 dma_addr_t phy_addr;
292
293 if (cmd && cmd->frame) {
294 if (cmd->frame->sg_counts) {
295 m_sg = (struct mvumi_sgl *)
&cmd->frame->payload[0];
296 sgd_getsz(mhba, m_sg, size);
297
298 phy_addr = (dma_addr_t) m_sg->baseaddr_l |
> 299 (dma_addr_t) m_sg->baseaddr_h << 32;
300
301 dma_free_coherent(&mhba->pdev->dev, size,
cmd->data_buf,
302
phy_addr);
303 }
304 dma_free_coherent(&mhba->pdev->dev, mhba->ib_max_size,
305 cmd->frame, cmd->frame_phys);
306 kfree(cmd);
307 }
308 }
309
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
On Tue, Aug 13, 2019 at 07:01:13PM +0100, Colin King wrote:
> From: Colin Ian King
>
> Currently the top 32 bits of a 64 bit address is being calculated
> by shifting a u32 twice by 16 bits and then being cast into a 64
> bit address. Shifting a u32 twice by 16 bits always ends up with
> a zero. Fix this by casting the u32 to a 64 bit address first
> and then shifting it 32 bits.
>
> Addresses-Coverity: ("Operands don't affect result")
> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> Signed-off-by: Colin Ian King
> ---
> drivers/scsi/mvumi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8906aceda4c4..62df69f1e71e 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -296,7 +296,7 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba
> *mhba,
> sgd_getsz(mhba, m_sg, size);
>
> phy_addr = (dma_addr_t) m_sg->baseaddr_l |
> - (dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
> +(dma_addr_t) m_sg->baseaddr_h << 32;
Colin, you've sent this patch before on Feb 16, 2019. If you shift by
32 then it's undefined behavior on 32 bit systems. The correct fix is
to move the cast which was what your original patch did actually.
(((dma_addr_t)m_sg->baseaddr_h << 16) << 16);
My suggestion back then was to introduce a macro:
/*
* The dma_addr_t type can be either 32 or 64 bit. Left shifting a 32
* bit number is undefined so this do two 16 bit left shifts.
*
*/
#define DMA_LSHIFT_32(val) (((dma_addr_t)(val) << 16) << 16)
regards,
dan carpenter
Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
Am 13.08.2019 20:01, schrieb Colin King:
> From: Colin Ian King
>
> Currently the top 32 bits of a 64 bit address is being calculated
> by shifting a u32 twice by 16 bits and then being cast into a 64
> bit address. Shifting a u32 twice by 16 bits always ends up with
> a zero. Fix this by casting the u32 to a 64 bit address first
> and then shifting it 32 bits.
>
> Addresses-Coverity: ("Operands don't affect result")
> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> Signed-off-by: Colin Ian King
> ---
> drivers/scsi/mvumi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index 8906aceda4c4..62df69f1e71e 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -296,7 +296,7 @@ static void mvumi_delete_internal_cmd(struct mvumi_hba
> *mhba,
> sgd_getsz(mhba, m_sg, size);
>
> phy_addr = (dma_addr_t) m_sg->baseaddr_l |
> - (dma_addr_t) ((m_sg->baseaddr_h << 16) << 16);
> +(dma_addr_t) m_sg->baseaddr_h << 32;
>
All the casts make it hard to read, i would propose an alternativ version:
phy_addr = m_sg->baseaddr_h;
phy_addr <<= 32;
phy_addr |= m_sg->baseaddr_l;
JM2C and totaly untested.
re,
wh
> dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
> phy_addr);
