From: Samiullah Khawaja <skhawaja@google.com>

Move the `dma_map` and `dma_sync` checks to `page_pool_init` to make
them generic. Set dma_sync to false for devmem memory provider because
the dma_sync APIs should not be used for dma_buf backed devmem memory
provider.

Cc: Jason Gunthorpe <jgg@ziepe.ca>
Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 net/core/devmem.c    | 9 ++++-----
 net/core/page_pool.c | 3 +++
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/core/devmem.c b/net/core/devmem.c
index 11b91c12ee11..826d0b00159f 100644
--- a/net/core/devmem.c
+++ b/net/core/devmem.c
@@ -331,11 +331,10 @@ int mp_dmabuf_devmem_init(struct page_pool *pool)
 	if (!binding)
 		return -EINVAL;
 
-	if (!pool->dma_map)
-		return -EOPNOTSUPP;
-
-	if (pool->dma_sync)
-		return -EOPNOTSUPP;
+	/* dma-buf dma addresses should not be used with
+	 * dma_sync_for_cpu/device. Force disable dma_sync.
+	 */
+	pool->dma_sync = false;
 
 	if (pool->p.order != 0)
 		return -E2BIG;
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 77de79c1933b..528dd4d18eab 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -287,6 +287,9 @@ static int page_pool_init(struct page_pool *pool,
 	}
 
 	if (pool->mp_priv) {
+		if (!pool->dma_map || !pool->dma_sync)
+			return -EOPNOTSUPP;
+
 		err = mp_dmabuf_devmem_init(pool);
 		if (err) {
 			pr_warn("%s() mem-provider init failed %d\n", __func__,
-- 
2.47.0.277.g8800431eea-goog

On Thu,  7 Nov 2024 21:23:07 +0000 Mina Almasry wrote:
> From: Samiullah Khawaja <skhawaja@google.com>
> 
> Move the `dma_map` and `dma_sync` checks to `page_pool_init` to make
> them generic. Set dma_sync to false for devmem memory provider because
> the dma_sync APIs should not be used for dma_buf backed devmem memory
> provider.

Let's start from specifying the expectations from the drivers,
and documenting them under Documentation/

We should require that drivers set PP_FLAG_DMA_SYNC_DEV on the page
pool and always use (a new form of) page_pool_dma_sync_for_cpu()
to sync for CPU.

Behind the scenes we will configure the PP to act accordingly.
For dmabuf backed PP I suspect we just say "syncing is
the responsibility of userspace", and configure the PP
to do no syncing at all.

For other providers we can keep the syncing enabled. But drivers
shouldn't be allowed to use any netmem if they don't set
PP_FLAG_DMA_SYNC_DEV, just to keep things uniform.

On 11/7/24 21:23, Mina Almasry wrote:
> From: Samiullah Khawaja <skhawaja@google.com>
> 
> Move the `dma_map` and `dma_sync` checks to `page_pool_init` to make
> them generic. Set dma_sync to false for devmem memory provider because
> the dma_sync APIs should not be used for dma_buf backed devmem memory
> provider.
> 
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>
> 
> ---
>   net/core/devmem.c    | 9 ++++-----
>   net/core/page_pool.c | 3 +++
>   2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index 11b91c12ee11..826d0b00159f 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c
> @@ -331,11 +331,10 @@ int mp_dmabuf_devmem_init(struct page_pool *pool)
>   	if (!binding)
>   		return -EINVAL;
>   
> -	if (!pool->dma_map)
> -		return -EOPNOTSUPP;
> -
> -	if (pool->dma_sync)
> -		return -EOPNOTSUPP;
> +	/* dma-buf dma addresses should not be used with
> +	 * dma_sync_for_cpu/device. Force disable dma_sync.
> +	 */
> +	pool->dma_sync = false;
>   
>   	if (pool->p.order != 0)
>   		return -E2BIG;
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 77de79c1933b..528dd4d18eab 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -287,6 +287,9 @@ static int page_pool_init(struct page_pool *pool,
>   	}
>   
>   	if (pool->mp_priv) {
> +		if (!pool->dma_map || !pool->dma_sync)

Is there a reason why ->dma_sync is enforced for all providers?
It sounds like a devmem fix, on the other hand I don't really
care as we don't have drivers yet implementing queue api / etc.
but not passing PP_FLAG_DMA_SYNC_DEV.

Also, putting fixes first in the series or sending them separately
reduces backporting headache.

-- 
Pavel Begunkov

On 11/07, Mina Almasry wrote:
> From: Samiullah Khawaja <skhawaja@google.com>
> 
> Move the `dma_map` and `dma_sync` checks to `page_pool_init` to make
> them generic. Set dma_sync to false for devmem memory provider because
> the dma_sync APIs should not be used for dma_buf backed devmem memory
> provider.
> 
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> Signed-off-by: Mina Almasry <almasrymina@google.com>

Acked-by: Stanislav Fomichev <sdf@fomichev.me>