Re: [PATCH v2 17/23] RDMA/uverbs: Use the SGT_P2P mapping type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] RDMA/uverbs: commit message contradicts
  uverbs_dmabuf_match_mapping()
--

commit f919774da72af458231453cd6f22c8076d245156
Author: Jason Gunthorpe <jgg@ziepe.ca>

RDMA/uverbs: Use the SGT_P2P mapping type

This commit converts uverbs dmabuf from an attach callback checking
attachment->peer2peer to a match_mapping callback using
DMA_BUF_EMAPPING_SGT_REQUIRES_P2P().

>     RDMA/uverbs: Use the SGT_P2P mapping type
>     
>     Very similar to habana, except the rejection of unsupported
>     pci_p2p_dma_distance() is done inside map not attach via
>     dma_buf_phys_vec_to_sgt().
>     
>     Convert this directly to a SGT_P2P match which will prevent matching if
>     dma_buf_phys_vec_to_sgt() would fail.
>     
>     Remove the now empty attach function.
>     
>     Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>     Signed-off-by: Alistair Popple <apopple@nvidia.com>
> 
> diff --git a/drivers/infiniband/core/uverbs_std_types_dmabuf.c b/drivers/infiniband/core/uverbs_std_types_dmabuf.c
> index 8d8c997a48834..0bae578c3d78e 100644
> --- a/drivers/infiniband/core/uverbs_std_types_dmabuf.c
> +++ b/drivers/infiniband/core/uverbs_std_types_dmabuf.c

[ ... ]

> @@ -95,13 +86,30 @@ static void uverbs_dmabuf_release(struct dma_buf *dmabuf)
>  	uverbs_uobject_put(&priv->uobj);
>  }
>  
> -static const struct dma_buf_ops uverbs_dmabuf_ops = {
> -	.attach = uverbs_dmabuf_attach,
> +static const struct dma_buf_mapping_sgt_exp_ops uverbs_dma_buf_sgt_ops = {
>  	.map_dma_buf = uverbs_dmabuf_map,
>  	.unmap_dma_buf = uverbs_dmabuf_unmap,
> +};
> +
> +static int uverbs_dmabuf_match_mapping(struct dma_buf_match_args *args)
> +{
> +	struct dma_buf_mapping_match sgt_match[1];
> +
> +	/*
> +	 * MMIO, so the importer must accept P2P. The provider is only valid
> +	 * under the dma_resv lock while not revoked, so the distance check is
> +	 * left to dma_buf_phys_vec_to_sgt() at map time, as before.
> +	 */
> +	sgt_match[0] = DMA_BUF_EMAPPING_SGT_REQUIRES_P2P(&uverbs_dma_buf_sgt_ops);

[Severity: Medium]
Does the commit message match the implementation in
uverbs_dmabuf_match_mapping()?

The commit title and changelog state that this converts uverbs to a SGT_P2P
match which prevents matching if dma_buf_phys_vec_to_sgt() would fail due to
an unsupported pci_p2p_dma_distance().

However, uverbs_dmabuf_match_mapping() uses
DMA_BUF_EMAPPING_SGT_REQUIRES_P2P() rather than DMA_BUF_EMAPPING_SGT_P2P().
As noted in the code comment, DMA_BUF_EMAPPING_SGT_REQUIRES_P2P() only checks
that the importer accepts P2P (replacing the !attachment->peer2peer check in
uverbs_dmabuf_attach()) and leaves the P2P distance check to
dma_buf_phys_vec_to_sgt() at map time because priv->provider is only valid
under the dma_resv lock while not revoked.

Should the commit title and message be updated to reflect that
DMA_BUF_EMAPPING_SGT_REQUIRES_P2P() is used instead of SGT_P2P, and that the
distance check remains at map time?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-dma_mapping_type_v4-v2-0-78b8dc9ae80a@nvidia.com?part=17