On Mon, Aug 14, 2023 at 08:55:44PM +0000, Saleem, Shiraz wrote:
> > Subject: Re: [PATCH -next] RDMA/irdma: Silence the warnings in
> > irdma_uk_rdma_write()
> >
> > On Fri, Aug 11, 2023 at 02:22:15PM +0800, Ruan Jinjie wrote:
> > > Remove sparse warnings introduced by commit 272bba19d631 ("RDMA:
> > > Remove unnecessary ternary operators"):
> > >
> > > drivers/infiniband/hw/irdma/uk.c:285:24: sparse: sparse: incorrect type in
> > assignment (different base types) @@ expected bool [usertype] push_wqe:1
> > @@ got restricted __le32 [usertype] *push_db @@
> > > drivers/infiniband/hw/irdma/uk.c:285:24: sparse: expected bool [usertype]
> > push_wqe:1
> > > drivers/infiniband/hw/irdma/uk.c:285:24: sparse: got restricted __le32
> > [usertype] *push_db
> > > drivers/infiniband/hw/irdma/uk.c:386:24: sparse: sparse: incorrect type in
> > assignment (different base types) @@ expected bool [usertype] push_wqe:1
> > @@ got restricted __le32 [usertype] *push_db @@
> > > drivers/infiniband/hw/irdma/uk.c:386:24: sparse: expected bool [usertype]
> > push_wqe:1
> > > drivers/infiniband/hw/irdma/uk.c:386:24: sparse: got restricted __le32
> > [usertype] *push_db
> > > drivers/infiniband/hw/irdma/uk.c:471:24: sparse: sparse: incorrect type in
> > assignment (different base types) @@ expected bool [usertype] push_wqe:1
> > @@ got restricted __le32 [usertype] *push_db @@
> > > drivers/infiniband/hw/irdma/uk.c:471:24: sparse: expected bool [usertype]
> > push_wqe:1
> > > drivers/infiniband/hw/irdma/uk.c:471:24: sparse: got restricted __le32
> > [usertype] *push_db
> > > drivers/infiniband/hw/irdma/uk.c:723:24: sparse: sparse: incorrect type in
> > assignment (different base types) @@ expected bool [usertype] push_wqe:1
> > @@ got restricted __le32 [usertype] *push_db @@
> > > drivers/infiniband/hw/irdma/uk.c:723:24: sparse: expected bool [usertype]
> > push_wqe:1
> > > drivers/infiniband/hw/irdma/uk.c:723:24: sparse: got restricted __le32
> > [usertype] *push_db
> > > drivers/infiniband/hw/irdma/uk.c:797:24: sparse: sparse: incorrect type in
> > assignment (different base types) @@ expected bool [usertype] push_wqe:1
> > @@ got restricted __le32 [usertype] *push_db @@
> > > drivers/infiniband/hw/irdma/uk.c:797:24: sparse: expected bool [usertype]
> > push_wqe:1
> > > drivers/infiniband/hw/irdma/uk.c:797:24: sparse: got restricted __le32
> > [usertype] *push_db
> > > drivers/infiniband/hw/irdma/uk.c:875:24: sparse: sparse: incorrect type in
> > assignment (different base types) @@ expected bool [usertype] push_wqe:1
> > @@ got restricted __le32 [usertype] *push_db @@
> > > drivers/infiniband/hw/irdma/uk.c:875:24: sparse: expected bool [usertype]
> > push_wqe:1
> > > drivers/infiniband/hw/irdma/uk.c:875:24: sparse: got restricted __le32
> > [usertype] *push_db
> > >
> > > Signed-off-by: Ruan Jinjie <ruanjinjie@xxxxxxxxxx>
> > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202308110251.BV6BcwUR-lkp@intel.
> > > com/
> > > ---
> > > drivers/infiniband/hw/irdma/uk.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/irdma/uk.c
> > > b/drivers/infiniband/hw/irdma/uk.c
> > > index a0739503140d..363c67c18924 100644
> > > --- a/drivers/infiniband/hw/irdma/uk.c
> > > +++ b/drivers/infiniband/hw/irdma/uk.c
> > > @@ -282,7 +282,7 @@ int irdma_uk_rdma_write(struct irdma_qp_uk *qp, struct
> > irdma_post_sq_info *info,
> > > bool read_fence = false;
> > > u16 quanta;
> > >
> > > - info->push_wqe = qp->push_db;
> > > + info->push_wqe = !!qp->push_db;
> >
> > Shiraz, push_db is declared as pointer, but I don't see where it is allocated. Current
> > code works because push_db is always 1 entry.
> >
> > 316 struct irdma_qp_uk {
> > ...
> > 324 __le32 *push_db;
> >
> > and
> >
> > 156 set_32bit_val(qp->push_db, 0,
> > 157 FIELD_PREP(IRDMA_WQEALLOC_WQE_DESC_INDEX,
> > wqe_idx >> 3) | qp->qp_id);
> >
> > Such variable use is not great. can you please fix it?
> > Can Ruan use "qp->push_mode" check instead of "qp->push_db"?
> >
>
> Hi Leon - Thanks for bring this to my attention.
>
> Seems we don't have all aspects of kernel push implementation and yes
> the push DB not mapped renders it void. And this code is also in kernel fast path :/
>
> kernel push is not plan of record at this point and the patch (below) cleans it up. I can send this to the mailing list.
>
> I am fine if we want to spot fix the sparse issue through Ruan's patch here.
> qp->push_mode and qp->push_db are not really equivalent.
> The latter is constant once db is mapped. The former is transient per qp, comes and goes.
> But its all mute anyway as it stands now.
>
> Or we can just use my removal patch to fix the sparse issue as well.
If we have an option to remove, it is always preferable solution to me.
Please send your removal patch to the ML.
Thanks
