* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-16 14:44 [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int Colin King
@ 2019-02-16 16:27 ` Walter Harms
2019-02-18 9:37 ` Dan Carpenter
2019-02-18 12:42 ` Walter Harms
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Walter Harms @ 2019-02-16 16:27 UTC (permalink / raw)
To: Colin King
Cc: Jianyun Li, James E . J . Bottomley, Martin K . Petersen,
linux-scsi, kernel-janitors, linux-kernel
Am 16.02.2019 15:44, schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being shifted by a
> total of 32 bits; this always produces a 0 result. Fix this by casting
> it to a dma_addr_t (a 64 bit unsigned int) before performing the shift.
>
> Detected by CoverityScan, CID#147270 ("Operands don't affect result")
>
> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> 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 36f64205ecfa..d3582accfd09 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -313,7 +313,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 << 16) << 16);
>
> dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
> phy_addr);
i would suggest to try a version with less casts to make it more readable
like this untested suggestion:
phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
phy_addr <<= 16;
just my 2 cents,
re,
wh
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-16 16:27 ` Walter Harms
@ 2019-02-18 9:37 ` Dan Carpenter
2019-02-18 15:32 ` James Bottomley
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2019-02-18 9:37 UTC (permalink / raw)
To: Walter Harms
Cc: Colin King, Jianyun Li, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel
On Sat, Feb 16, 2019 at 05:27:16PM +0100, Walter Harms wrote:
> Am 16.02.2019 15:44, schrieb Colin King:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being shifted by a
> > total of 32 bits; this always produces a 0 result. Fix this by casting
> > it to a dma_addr_t (a 64 bit unsigned int) before performing the shift.
> >
> > Detected by CoverityScan, CID#147270 ("Operands don't affect result")
> >
> > Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > 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 36f64205ecfa..d3582accfd09 100644
> > --- a/drivers/scsi/mvumi.c
> > +++ b/drivers/scsi/mvumi.c
> > @@ -313,7 +313,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 << 16) << 16);
> >
> > dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
> > phy_addr);
>
> i would suggest to try a version with less casts to make it more readable
> like this untested suggestion:
>
> phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
> phy_addr <<= 16;
>
That would be a behavior change but it also might be a bugfix? Why
doesn't the code just do:
phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l;
(Probably they broke it up into two shifts to silence a GCC warning that
the shift was wrong because of the missing cast?)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-18 9:37 ` Dan Carpenter
@ 2019-02-18 15:32 ` James Bottomley
2019-02-18 15:47 ` Dan Carpenter
0 siblings, 1 reply; 12+ messages in thread
From: James Bottomley @ 2019-02-18 15:32 UTC (permalink / raw)
To: Dan Carpenter, Walter Harms
Cc: Colin King, Jianyun Li, Martin K . Petersen, linux-scsi,
kernel-janitors, linux-kernel
On Mon, 2019-02-18 at 12:37 +0300, Dan Carpenter wrote:
> On Sat, Feb 16, 2019 at 05:27:16PM +0100, Walter Harms wrote:
> > Am 16.02.2019 15:44, schrieb Colin King:
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being
> > > shifted by a
> > > total of 32 bits; this always produces a 0 result. Fix this by
> > > casting
> > > it to a dma_addr_t (a 64 bit unsigned int) before performing the
> > > shift.
> > >
> > > Detected by CoverityScan, CID#147270 ("Operands don't affect
> > > result")
> > >
> > > Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > > 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 36f64205ecfa..d3582accfd09 100644
> > > --- a/drivers/scsi/mvumi.c
> > > +++ b/drivers/scsi/mvumi.c
> > > @@ -313,7 +313,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
> > > << 16) << 16);
> > >
> > > dma_free_coherent(&mhba->pdev->dev,
> > > size, cmd->data_buf,
> > >
> > > phy_addr);
> >
> > i would suggest to try a version with less casts to make it more
> > readable
> > like this untested suggestion:
> >
> > phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
> > phy_addr <<= 16;
> >
>
> That would be a behavior change but it also might be a bugfix? Why
> doesn't the code just do:
>
> phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg-
> >baseaddr_l;
>
> (Probably they broke it up into two shifts to silence a GCC warning
> that the shift was wrong because of the missing cast?)
No because dma_addr_t can be 32 bits and the warning would then always
appear on some builds. The << 16 << 16 makes sure it doesn't.
James
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-18 15:32 ` James Bottomley
@ 2019-02-18 15:47 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2019-02-18 15:47 UTC (permalink / raw)
To: James Bottomley
Cc: Walter Harms, Colin King, Jianyun Li, Martin K . Petersen,
linux-scsi, kernel-janitors, linux-kernel
On Mon, Feb 18, 2019 at 07:32:05AM -0800, James Bottomley wrote:
> On Mon, 2019-02-18 at 12:37 +0300, Dan Carpenter wrote:
> > On Sat, Feb 16, 2019 at 05:27:16PM +0100, Walter Harms wrote:
> > > Am 16.02.2019 15:44, schrieb Colin King:
> > > > From: Colin Ian King <colin.king@canonical.com>
> > > >
> > > > Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being
> > > > shifted by a
> > > > total of 32 bits; this always produces a 0 result. Fix this by
> > > > casting
> > > > it to a dma_addr_t (a 64 bit unsigned int) before performing the
> > > > shift.
> > > >
> > > > Detected by CoverityScan, CID#147270 ("Operands don't affect
> > > > result")
> > > >
> > > > Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> > > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > > ---
> > > > 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 36f64205ecfa..d3582accfd09 100644
> > > > --- a/drivers/scsi/mvumi.c
> > > > +++ b/drivers/scsi/mvumi.c
> > > > @@ -313,7 +313,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
> > > > << 16) << 16);
> > > >
> > > > dma_free_coherent(&mhba->pdev->dev,
> > > > size, cmd->data_buf,
> > > >
> > > > phy_addr);
> > >
> > > i would suggest to try a version with less casts to make it more
> > > readable
> > > like this untested suggestion:
> > >
> > > phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
> > > phy_addr <<= 16;
> > >
> >
> > That would be a behavior change but it also might be a bugfix? Why
> > doesn't the code just do:
> >
> > phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg-
> > >baseaddr_l;
> >
> > (Probably they broke it up into two shifts to silence a GCC warning
> > that the shift was wrong because of the missing cast?)
>
> No because dma_addr_t can be 32 bits and the warning would then always
> appear on some builds. The << 16 << 16 makes sure it doesn't.
>
Yeah. You're right. Thanks.
The original patch is the right fix. Although it sort of feels like the
double shift should be 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-16 14:44 [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int Colin King
2019-02-16 16:27 ` Walter Harms
@ 2019-02-18 12:42 ` Walter Harms
2019-02-18 14:24 ` Dan Carpenter
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Walter Harms @ 2019-02-18 12:42 UTC (permalink / raw)
To: kernel-janitors
Am 18.02.2019 10:37, schrieb Dan Carpenter:
> On Sat, Feb 16, 2019 at 05:27:16PM +0100, Walter Harms wrote:
>> Am 16.02.2019 15:44, schrieb Colin King:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being shifted by a
>>> total of 32 bits; this always produces a 0 result. Fix this by casting
>>> it to a dma_addr_t (a 64 bit unsigned int) before performing the shift.
>>>
>>> Detected by CoverityScan, CID#147270 ("Operands don't affect result")
>>>
>>> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>> 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 36f64205ecfa..d3582accfd09 100644
>>> --- a/drivers/scsi/mvumi.c
>>> +++ b/drivers/scsi/mvumi.c
>>> @@ -313,7 +313,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 << 16) << 16);
>>>
>>> dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
>>> phy_addr);
>>
>> i would suggest to try a version with less casts to make it more readable
>> like this untested suggestion:
>>
>> phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
>> phy_addr <<= 16;
>>
>
> That would be a behavior change but it also might be a bugfix? Why
> doesn't the code just do:
>
> phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l;
>
phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l<<16;
The question is how to solv propperly this without confusing the future reader.
Lets wait what opinion the maintainer has.
re,
wh
> (Probably they broke it up into two shifts to silence a GCC warning that
> the shift was wrong because of the missing cast?)
>
> regards,
> dan carpenter
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-16 14:44 [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int Colin King
2019-02-16 16:27 ` Walter Harms
2019-02-18 12:42 ` Walter Harms
@ 2019-02-18 14:24 ` Dan Carpenter
2019-02-18 16:52 ` Walter Harms
2019-08-13 18:01 ` [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value Colin King
4 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2019-02-18 14:24 UTC (permalink / raw)
To: kernel-janitors
On Mon, Feb 18, 2019 at 01:42:02PM +0100, Walter Harms wrote:
> Am 18.02.2019 10:37, schrieb Dan Carpenter:
> > On Sat, Feb 16, 2019 at 05:27:16PM +0100, Walter Harms wrote:
> >> Am 16.02.2019 15:44, schrieb Colin King:
> >>> From: Colin Ian King <colin.king@canonical.com>
> >>>
> >>> Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being shifted by a
> >>> total of 32 bits; this always produces a 0 result. Fix this by casting
> >>> it to a dma_addr_t (a 64 bit unsigned int) before performing the shift.
> >>>
> >>> Detected by CoverityScan, CID#147270 ("Operands don't affect result")
> >>>
> >>> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >>> ---
> >>> 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 36f64205ecfa..d3582accfd09 100644
> >>> --- a/drivers/scsi/mvumi.c
> >>> +++ b/drivers/scsi/mvumi.c
> >>> @@ -313,7 +313,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 << 16) << 16);
> >>>
> >>> dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
> >>> phy_addr);
> >>
> >> i would suggest to try a version with less casts to make it more readable
> >> like this untested suggestion:
> >>
> >> phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
> >> phy_addr <<= 16;
> >>
> >
> > That would be a behavior change but it also might be a bugfix? Why
> > doesn't the code just do:
> >
> > phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l;
> >
>
> phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l<<16;
^^^^
In the original code, we don't do this shift.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int
2019-02-16 14:44 [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int Colin King
` (2 preceding siblings ...)
2019-02-18 14:24 ` Dan Carpenter
@ 2019-02-18 16:52 ` Walter Harms
2019-08-13 18:01 ` [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value Colin King
4 siblings, 0 replies; 12+ messages in thread
From: Walter Harms @ 2019-02-18 16:52 UTC (permalink / raw)
To: kernel-janitors
Am 18.02.2019 15:24, schrieb Dan Carpenter:
> On Mon, Feb 18, 2019 at 01:42:02PM +0100, Walter Harms wrote:
>> Am 18.02.2019 10:37, schrieb Dan Carpenter:
>>> On Sat, Feb 16, 2019 at 05:27:16PM +0100, Walter Harms wrote:
>>>> Am 16.02.2019 15:44, schrieb Colin King:
>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>
>>>>> Currently m_sg->baseaddr_h (a 32 bit unsigned int) is being shifted by a
>>>>> total of 32 bits; this always produces a 0 result. Fix this by casting
>>>>> it to a dma_addr_t (a 64 bit unsigned int) before performing the shift.
>>>>>
>>>>> Detected by CoverityScan, CID#147270 ("Operands don't affect result")
>>>>>
>>>>> Fixes: f0c568a478f0 ("[SCSI] mvumi: Add Marvell UMI driver")
>>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>>> ---
>>>>> 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 36f64205ecfa..d3582accfd09 100644
>>>>> --- a/drivers/scsi/mvumi.c
>>>>> +++ b/drivers/scsi/mvumi.c
>>>>> @@ -313,7 +313,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 << 16) << 16);
>>>>>
>>>>> dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
>>>>> phy_addr);
>>>>
>>>> i would suggest to try a version with less casts to make it more readable
>>>> like this untested suggestion:
>>>>
>>>> phy_addr =(m_sg->baseaddr_h << 16)| m_sg->baseaddr_l;
>>>> phy_addr <<= 16;
>>>>
>>>
>>> That would be a behavior change but it also might be a bugfix? Why
>>> doesn't the code just do:
>>>
>>> phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l;
>>>
>>
>> phy_addr = ((dma_addr_t)m_sg->baseaddr_h << 32) | m_sg->baseaddr_l<<16;
> ^^^^
> In the original code, we don't do this shift.
>
You are right, my bad, sorry
my brain made it into 16bit values.
on the other side it shows my point that things must be obvious as possible,
re,
wh
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
2019-02-16 14:44 [PATCH] scsi: mvumi: fix 32 bit shift of a 32 bit unsigned int Colin King
` (3 preceding siblings ...)
2019-02-18 16:52 ` Walter Harms
@ 2019-08-13 18:01 ` Colin King
2019-08-14 7:17 ` walter harms
` (2 more replies)
4 siblings, 3 replies; 12+ messages in thread
From: Colin King @ 2019-08-13 18:01 UTC (permalink / raw)
To: Jianyun Li, James E . J . Bottomley, Martin K . Petersen,
linux-scsi
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
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 <colin.king@canonical.com>
---
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;
dma_free_coherent(&mhba->pdev->dev, size, cmd->data_buf,
phy_addr);
--
2.20.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
2019-08-13 18:01 ` [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value Colin King
@ 2019-08-14 7:17 ` walter harms
2019-08-14 8:07 ` Dan Carpenter
2019-08-14 15:07 ` kbuild test robot
2 siblings, 0 replies; 12+ messages in thread
From: walter harms @ 2019-08-14 7:17 UTC (permalink / raw)
To: Colin King
Cc: Jianyun Li, James E . J . Bottomley, Martin K . Petersen,
linux-scsi, kernel-janitors, linux-kernel
Am 13.08.2019 20:01, schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
>
> 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 <colin.king@canonical.com>
> ---
> 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);
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
2019-08-13 18:01 ` [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value Colin King
2019-08-14 7:17 ` walter harms
@ 2019-08-14 8:07 ` Dan Carpenter
2019-08-14 15:07 ` kbuild test robot
2 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2019-08-14 8:07 UTC (permalink / raw)
To: Colin King
Cc: Jianyun Li, James E . J . Bottomley, Martin K . Petersen,
linux-scsi, kernel-janitors, linux-kernel
On Tue, Aug 13, 2019 at 07:01:13PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> 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 <colin.king@canonical.com>
> ---
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value
2019-08-13 18:01 ` [PATCH] scsi: mvumi: fix 32 bit shift of a u32 value Colin King
2019-08-14 7:17 ` walter harms
2019-08-14 8:07 ` Dan Carpenter
@ 2019-08-14 15:07 ` kbuild test robot
2 siblings, 0 replies; 12+ messages in thread
From: kbuild test robot @ 2019-08-14 15:07 UTC (permalink / raw)
To: Colin King
Cc: kbuild-all, Jianyun Li, James E . J . Bottomley,
Martin K . Petersen, linux-scsi, kernel-janitors, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]
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 <lkp@intel.com>
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 infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61472 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread