On Mon, 2017-07-31 at 16:57 +0000, Bart Van Assche wrote:
> On Mon, 2017-07-31 at 19:50 +0300, Leon Romanovsky wrote:
> > On Mon, Jul 31, 2017 at 04:23:52PM +0000, Bart Van Assche wrote:
> > > On Mon, 2017-07-31 at 19:16 +0300, Leon Romanovsky wrote:
> > > > On Mon, Jul 31, 2017 at 03:17:24PM +0000, Bart Van Assche wrote:
> > > > > On Mon, 2017-07-31 at 10:09 +0300, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > >
> > > > > > The "lg" variable is declared as int so in all places where
> > > > > > this variable is used as a shift operand, the output will be
> > > > > > int too.
> > > > > >
> > > > > > This produces the following smatch warning:
> > > > > > drivers/infiniband/hw/mthca/mthca_cmd.c:701 mthca_map_cmd() warn:
> > > > > > should '1 << lg' be a 64 bit type?
> > > > > >
> > > > > > Simple declaration of "1" to be "1ULL" will fix the issue.
> > > > > >
> > > > > > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> > > > > > Signed-off-by: Leon Romanovsky <leon@xxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/infiniband/hw/mthca/mthca_cmd.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/hw/mthca/mthca_cmd.c b/drivers/infiniband/hw/mthca/mthca_cmd.c
> > > > > > index 9d83a53c0c67..1052c35f2e75 100644
> > > > > > --- a/drivers/infiniband/hw/mthca/mthca_cmd.c
> > > > > > +++ b/drivers/infiniband/hw/mthca/mthca_cmd.c
> > > > > > @@ -698,7 +698,7 @@ static int mthca_map_cmd(struct mthca_dev *dev, u16 op, struct mthca_icm *icm,
> > > > > > for (i = 0; i < mthca_icm_size(&iter) >> lg; ++i) {
> > > > > > if (virt != -1) {
> > > > > > pages[nent * 2] = cpu_to_be64(virt);
> > > > > > - virt += 1 << lg;
> > > > > > + virt += 1ULL << lg;
> > > > > > }
> > > > > >
> > > > > > pages[nent * 2 + 1] =
> > > > >
> > > > > Hello Leon,
> > > > >
> > > > > Is my analysis correct that lg can be equal to or larger than 32? If so,
> > > > > isn't this patch a bug fix that needs a "Fixes:" tag?
> > > >
> > > > This is the fix to commit introduced in pre-git era and I don't have
> > > > adequate Fixes tag. The fact that no one complained about the problem
> > > > suggests to me that no one is really important to have it in stable trees.
> > > >
> > > > Tariq and me did the analysis for the same code issue with mlx4 (he is
> > > > planning to forward the fix to Dave) and our conclusion that it is not
> > > > bug.
> > > >
> > > > The reason to it in line 689:drivers/infiniband/hw/mthca/mthca_cmd.c
> > > > 689 lg = ffs(mthca_icm_addr(&iter) | mthca_icm_size(&iter)) - 1;
> > > >
> > > > The "lg" for sure will be less than 31, because ffs returns values from 0 to 31 and minus 1.
> > > > The situation that ffs will return 0 is unlikely to happen.
> > >
> > > Hello Leon,
> > >
> > > mthca_icm_addr() returns a DMA address (dma_addr_t). Does that mean that
> > > that function can return a 64-bit value on 64-bit systems? Shouldn't ffs()
> > > be changed into a function that supports 64-bit values?
> >
> > Don't dismiss the second part of that ffs: "| mthca_icm_size(&iter)", to
> > overflow ffs, the icm size should be more than 2^32. Is it real scenario?
>
> The second part of the logical or expression is not relevant if the first part
> can be equal to or larger than 2^32. Has the mthca driver ever been tested on
> a 64-bit system or should it perhaps be marked as not supporting 64-bit systems?
Sorry - I confused ffs() with another instruction. Please ignore my reply.
Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
