Re: [PATCH 2/2] RDMA: dma-mapping: Return an unsigned int from ib_dma_map_sg{,_attrs}

[Leon Romanovsky]

On Mon, Aug 29, 2022 at 03:19:14PM +0200, Jinpu Wang wrote:
> On Mon, Aug 29, 2022 at 2:06 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> >
> > On Mon, Aug 29, 2022 at 11:40:40AM +0200, Jinpu Wang wrote:
> > > On Sun, Aug 28, 2022 at 1:09 PM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Aug 26, 2022 at 11:56:15AM +0200, Jack Wang wrote:
> > > > > Following 2a047e0662ae ("dma-mapping: return an unsigned int from dma_map_sg{,_attrs}")
> > > > > change the return value of ib_dma_map_sg{,attrs} to unsigned int.
> > > > >
> > > > > Cc: Jason Gunthorpe <jgg@xxxxxxxx>
> > > > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>
> > > > > Cc: Christoph Hellwig <hch@xxxxxx>
> > > > > Cc: linux-rdma@xxxxxxxxxxxxxxx
> > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > > >
> > > > > Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxx>
> > > > > ---
> > > > >  drivers/infiniband/core/device.c | 2 +-
> > > > >  include/rdma/ib_verbs.h          | 6 +++---
> > > > >  2 files changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > You forgot to change ib_dma_map_sgtable_attrs() and various
> > > > ib_dma_map_sg*() callers.
> > > No, they are different.
> > > ib_dma_map_sgtable_attrs and dma_map_sgtable return negative on errors.
> >
> > It is not the point. You changed ib_dma_virt_map_sg() to be unsigned,
> > so now the following lines are not correct:
> >
> >   4138         int nents;
> >   4139
> >   4140         if (ib_uses_virt_dma(dev)) {
> >   4141                 nents = ib_dma_virt_map_sg(dev, sgt->sgl, sgt->orig_nents);
> >
> > "int nents" should be changed to "unsigned int".
> >
> > Thanks
> ok, I can do it.
> just to check if we are on the same page:
> For all the callers of ib_dma_map_sg,  would it be better to fix it
> one patch per driver or do it in a single bigger patch.
> I feel it's better to do it per driver, and there are drivers from
> different subsystems e.g. nvme/rds/etc.

I don't know, everything here looks not nice to me.

After commit 2a047e0662ae ("dma-mapping: return an unsigned int from dma_map_sg{,_attrs}"),
all callers left in limbo state where they expect that dma_map_sg{,_attrs} will return
upto INT_MAX. However now, the API can return upto UINT_MAX, which is not the case now
due to internal implementation of dma_map_sg_*(), but can be changed any second.

Can we simply revert that commit and restore the "int" return type?
I don't see any benefit in having "unsigned int" if compiler doesn't enforce it.

Thanks

> 
> Thx!
> 
> 
> >
> > > >
> > > > Thanks
> > > Thanks!