[PATCH 0/2] infiniband: Fxi dma_map_sg error check

[PATCH 0/2] infiniband: Fxi dma_map_sg error check

[Jack Wang]

Hi, all,

While working on a bugfix on RTRS[1], I noticed there are quite a few other
drivers have the same problem, due to the fact dma_map_sg return 0 on error,
not like most of the cases, return negative value for error.

I "grep -A 5 dma_map_sg' in kernel tree, and audit/fix the one I feel is buggy,
hence this patchset. As suggested by Christoph Hellwig, I now send the patches per
subsystem, this is for infiniband subsystem.

The first one for mthca, leon mentioned it's too old to change, I still keep it
in the hope others have a different opinion.

The second patch change the return value follow the patch from Christoph
Hellwig, we might want to change the type for in the drivers which calls
dma_map_sg or ib_dma_map_sg to unsigned int to.

Thanks!

[1] https://lore.kernel.org/linux-rdma/20220818105355.110344-1-haris.iqbal@ionos.com/T/#t
Jack Wang (2):
  infiniband/mthca: Fix dma_map_sg error check
  RDMA: dma-mapping: Return an unsigned int from ib_dma_map_sg{,_attrs}

 drivers/infiniband/core/device.c            | 2 +-
 drivers/infiniband/hw/mthca/mthca_memfree.c | 7 ++++---
 include/rdma/ib_verbs.h                     | 6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] infiniband/mthca: Fix dma_map_sg error check

[Jack Wang]

dma_map_sg return 0 on error, in case of error set
EIO as return code.

Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Kees Cook <keescook@chromium.org>
Cc: "Håkon Bugge" <haakon.bugge@oracle.com>
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Fixes: 56483ec1b702 ("[PATCH] IB uverbs: add mthca user doorbell record support")
Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
 drivers/infiniband/hw/mthca/mthca_memfree.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/mthca/mthca_memfree.c b/drivers/infiniband/hw/mthca/mthca_memfree.c
index f2734a5c5f26..44fd5fdf64d5 100644
--- a/drivers/infiniband/hw/mthca/mthca_memfree.c
+++ b/drivers/infiniband/hw/mthca/mthca_memfree.c
@@ -189,7 +189,7 @@ struct mthca_icm *mthca_alloc_icm(struct mthca_dev *dev, int npages,
 						   chunk->npages,
 						   DMA_BIDIRECTIONAL);
 
-				if (chunk->nsg <= 0)
+				if (!chunk->nsg)
 					goto fail;
 			}
 
@@ -208,7 +208,7 @@ struct mthca_icm *mthca_alloc_icm(struct mthca_dev *dev, int npages,
 		chunk->nsg = dma_map_sg(&dev->pdev->dev, chunk->mem,
 					chunk->npages, DMA_BIDIRECTIONAL);
 
-		if (chunk->nsg <= 0)
+		if (!chunk->nsg)
 			goto fail;
 	}
 
@@ -482,8 +482,9 @@ int mthca_map_user_db(struct mthca_dev *dev, struct mthca_uar *uar,
 
 	ret = dma_map_sg(&dev->pdev->dev, &db_tab->page[i].mem, 1,
 			 DMA_TO_DEVICE);
-	if (ret < 0) {
+	if (!ret) {
 		unpin_user_page(pages[0]);
+		ret = -EIO;
 		goto out;
 	}
 
-- 
2.34.1

Re: [PATCH 1/2] infiniband/mthca: Fix dma_map_sg error check

[Leon Romanovsky]

On Fri, Aug 26, 2022 at 11:56:14AM +0200, Jack Wang wrote:
> dma_map_sg return 0 on error, in case of error set
> EIO as return code.
> 
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Håkon Bugge" <haakon.bugge@oracle.com>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Fixes: 56483ec1b702 ("[PATCH] IB uverbs: add mthca user doorbell record support")
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
>  drivers/infiniband/hw/mthca/mthca_memfree.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Same answer as was here
https://lore.kernel.org/all/YwIbI3ktmEiLsy6s@unreal

Thanks

Re: [PATCH 1/2] infiniband/mthca: Fix dma_map_sg error check

[Jinpu Wang]

On Sun, Aug 28, 2022 at 12:10 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Aug 26, 2022 at 11:56:14AM +0200, Jack Wang wrote:
> > dma_map_sg return 0 on error, in case of error set
> > EIO as return code.
> >
> > Cc: Jason Gunthorpe <jgg@ziepe.ca>
> > Cc: Leon Romanovsky <leon@kernel.org>
> > Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: "Håkon Bugge" <haakon.bugge@oracle.com>
> > Cc: linux-rdma@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Fixes: 56483ec1b702 ("[PATCH] IB uverbs: add mthca user doorbell record support")
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > ---
> >  drivers/infiniband/hw/mthca/mthca_memfree.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
>
> Same answer as was here
> https://lore.kernel.org/all/YwIbI3ktmEiLsy6s@unreal
>
> Thanks
ok, I see you are firm on this, we can skip the patch

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

[Jack Wang]

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@ziepe.ca>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: linux-rdma@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
---
 drivers/infiniband/core/device.c | 2 +-
 include/rdma/ib_verbs.h          | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index d275db195f1a..72489294391d 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2721,7 +2721,7 @@ void ib_set_device_ops(struct ib_device *dev, const struct ib_device_ops *ops)
 EXPORT_SYMBOL(ib_set_device_ops);
 
 #ifdef CONFIG_INFINIBAND_VIRT_DMA
-int ib_dma_virt_map_sg(struct ib_device *dev, struct scatterlist *sg, int nents)
+unsigned int ib_dma_virt_map_sg(struct ib_device *dev, struct scatterlist *sg, int nents)
 {
 	struct scatterlist *s;
 	int i;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 975d6e9efbcb..49256bf8cbf5 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -4101,8 +4101,8 @@ static inline void ib_dma_unmap_page(struct ib_device *dev,
 		dma_unmap_page(dev->dma_device, addr, size, direction);
 }
 
-int ib_dma_virt_map_sg(struct ib_device *dev, struct scatterlist *sg, int nents);
-static inline int ib_dma_map_sg_attrs(struct ib_device *dev,
+unsigned int ib_dma_virt_map_sg(struct ib_device *dev, struct scatterlist *sg, int nents);
+static inline unsigned int ib_dma_map_sg_attrs(struct ib_device *dev,
 				      struct scatterlist *sg, int nents,
 				      enum dma_data_direction direction,
 				      unsigned long dma_attrs)
@@ -4163,7 +4163,7 @@ static inline void ib_dma_unmap_sgtable_attrs(struct ib_device *dev,
  * @nents: The number of scatter/gather entries
  * @direction: The direction of the DMA
  */
-static inline int ib_dma_map_sg(struct ib_device *dev,
+static inline unsigned int ib_dma_map_sg(struct ib_device *dev,
 				struct scatterlist *sg, int nents,
 				enum dma_data_direction direction)
 {
-- 
2.34.1

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

[Leon Romanovsky]

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@ziepe.ca>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: linux-rdma@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  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.

Thanks

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

[Jinpu Wang]

On Sun, Aug 28, 2022 at 1:09 PM Leon Romanovsky <leon@kernel.org> 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@ziepe.ca>
> > Cc: Leon Romanovsky <leon@kernel.org>
> > Cc: Christoph Hellwig <hch@lst.de>
> > Cc: linux-rdma@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> >
> > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > ---
> >  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.
>
> Thanks
Thanks!

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 11:40:40AM +0200, Jinpu Wang wrote:
> On Sun, Aug 28, 2022 at 1:09 PM Leon Romanovsky <leon@kernel.org> 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@ziepe.ca>
> > > Cc: Leon Romanovsky <leon@kernel.org>
> > > Cc: Christoph Hellwig <hch@lst.de>
> > > Cc: linux-rdma@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > >
> > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > ---
> > >  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

> >
> > Thanks
> Thanks!

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

[Jinpu Wang]

On Mon, Aug 29, 2022 at 2:06 PM Leon Romanovsky <leon@kernel.org> 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@kernel.org> 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@ziepe.ca>
> > > > Cc: Leon Romanovsky <leon@kernel.org>
> > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > Cc: linux-rdma@vger.kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > >
> > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > > ---
> > > >  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.

Thx!


>
> > >
> > > Thanks
> > Thanks!

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@kernel.org> 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@kernel.org> 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@ziepe.ca>
> > > > > Cc: Leon Romanovsky <leon@kernel.org>
> > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > Cc: linux-rdma@vger.kernel.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > >
> > > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > > > ---
> > > > >  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!

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

[Jinpu Wang]

On Tue, Aug 30, 2022 at 10:01 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> 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@kernel.org> 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@kernel.org> 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@ziepe.ca>
> > > > > > Cc: Leon Romanovsky <leon@kernel.org>
> > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > Cc: linux-rdma@vger.kernel.org
> > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > >
> > > > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > > > > ---
> > > > > >  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.
I feel different, the dma_map_sg api since the kernel 2.x, is
documented in DMA-API.txt[1]:
"

int
dma_map_sg(struct device *dev, struct scatterlist *sg,
int nents, enum dma_data_direction direction)

Returns: the number of physical segments mapped (this may be shorter
than <nents> passed in if some elements of the scatter/gather list are
physically or virtually adjacent and an IOMMU maps them with a single
entry).

Please note that the sg cannot be mapped again if it has been mapped once.
The mapping process is allowed to destroy information in the sg.

As with the other mapping interfaces, dma_map_sg can fail. When it
does, 0 is returned and a driver must take appropriate action. It is
critical that the driver do something, in the case of a block driver
aborting the request or even oopsing is better than doing nothing and
corrupting the filesystem.

"
It seems the return range for dma_map_sg never returns a negative
value. I think it's just the API
should have been defined to return "unsigned int"  IMHO. We should
update the documentation in the Documentation there
too. in core-api/dma-api.rst



[1] https://elixir.bootlin.com/linux/v2.6.39.4/source/Documentation/DMA-API.txt


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

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

[Leon Romanovsky]

On Tue, Aug 30, 2022 at 10:23:46AM +0200, Jinpu Wang wrote:
> On Tue, Aug 30, 2022 at 10:01 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > 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@kernel.org> 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@kernel.org> 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@ziepe.ca>
> > > > > > > Cc: Leon Romanovsky <leon@kernel.org>
> > > > > > > Cc: Christoph Hellwig <hch@lst.de>
> > > > > > > Cc: linux-rdma@vger.kernel.org
> > > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > >
> > > > > > > Signed-off-by: Jack Wang <jinpu.wang@ionos.com>
> > > > > > > ---
> > > > > > >  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.
> I feel different, the dma_map_sg api since the kernel 2.x, is
> documented in DMA-API.txt[1]:
> "
> 
> int
> dma_map_sg(struct device *dev, struct scatterlist *sg,
> int nents, enum dma_data_direction direction)
> 
> Returns: the number of physical segments mapped (this may be shorter
> than <nents> passed in if some elements of the scatter/gather list are
> physically or virtually adjacent and an IOMMU maps them with a single
> entry).
> 
> Please note that the sg cannot be mapped again if it has been mapped once.
> The mapping process is allowed to destroy information in the sg.
> 
> As with the other mapping interfaces, dma_map_sg can fail. When it
> does, 0 is returned and a driver must take appropriate action. It is
> critical that the driver do something, in the case of a block driver
> aborting the request or even oopsing is better than doing nothing and
> corrupting the filesystem.
> 
> "
> It seems the return range for dma_map_sg never returns a negative
> value. I think it's just the API
> should have been defined to return "unsigned int"  IMHO. We should
> update the documentation in the Documentation there
> too. in core-api/dma-api.rst

If you need documentation and implementation to use API, it is not best API [1].
According to Rusty's manifesto it is "2. Read the implementation and you'll get it right.".

You need to dig into the function to understand that UINT_MAX is not
possible, instead of relying on compiler that will prevent such number
if callers are not updated to be unsigned int safe.

So commit 2a047e0662ae "downgraded" API from level "3. Read the documentation and
you'll get it right." to level 2.

Thanks

[1] http://sweng.the-davies.net/Home/rustys-api-design-manifesto

> 
> 
> 
> [1] https://elixir.bootlin.com/linux/v2.6.39.4/source/Documentation/DMA-API.txt
> 
> 
> >
> > Thanks
> >
> > >
> > > Thx!
> > >
> > >
> > > >
> > > > > >
> > > > > > Thanks
> > > > > Thanks!