Re: [PATCH v5 RFC 1/6] page_pool: frag API support for 32-bit arch with 64-bit DMA

[Jakub Kicinski]

On Sun, 9 Jul 2023 20:39:45 +0800 Yunsheng Lin wrote:
> On 2023/7/8 7:59, Jakub Kicinski wrote:
> > On Thu, 29 Jun 2023 20:02:21 +0800 Yunsheng Lin wrote:  
> >> +		/* Return error here to avoid mlx5e_page_release_fragmented()
> >> +		 * calling page_pool_defrag_page() to write to pp_frag_count
> >> +		 * which is overlapped with dma_addr_upper in 'struct page' for
> >> +		 * arch with PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true.
> >> +		 */
> >> +		if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT) {
> >> +			err = -EINVAL;
> >> +			goto err_free_by_rq_type;
> >> +		}  
> > 
> > I told you not to do this in a comment on v4.
> > Keep the flag in page pool params and let the creation fail.  
> 
> There seems to be naming disagreement in the previous discussion,
> The simplest way seems to be reuse the
> PAGE_POOL_DMA_USE_PP_FRAG_COUNT and do the checking in the driver
> without introducing new macro or changing macro name.
> 
> Let's be more specific about what is your suggestion here:
> Do you mean keep the PP_FLAG_PAGE_FRAG flag and keep the below
> checking in page_pool_init(), right?
> 	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> 	    pool->p.flags & PP_FLAG_PAGE_FRAG)
> 		return -EINVAL;
> 
> Isn't it confusing to still say page frag is not supported
> for PAGE_POOL_DMA_USE_PP_FRAG_COUNT being true case when this
> patch will now add support for it, at least from API POV, the
> page_pool_alloc_frag() is always supported now.

I don't mind what the flag is called, I just want the check to stay
inside the page_pool code, acting on driver info passed inside
pp_params.

> Maybe remove the PP_FLAG_PAGE_FRAG and add a new macro named
> PP_FLAG_PAGE_SPLIT_IN_DRIVER, and do the checking as before in
> page_pool_init() like below?
> 	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
> 	    pool->p.flags & PP_FLAG_PAGE_SPLIT_IN_DRIVER)
> 		return -EINVAL;

Yup, that sound good.