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.
