netdev - Re: [PATCH net-next 5/9] page_pool: don't use driver-set flags field directly

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 28 Jul 2023 16:03:53 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Yunsheng Lin <linyunsheng@...wei.com>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
	Larysa Zaremba <larysa.zaremba@...el.com>, Alexander Duyck
	<alexanderduyck@...com>, Jesper Dangaard Brouer <hawk@...nel.org>, "Ilias
 Apalodimas" <ilias.apalodimas@...aro.org>, Simon Horman
	<simon.horman@...igine.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 5/9] page_pool: don't use driver-set flags field
 directly

From: Yunsheng Lin <linyunsheng@...wei.com>
Date: Fri, 28 Jul 2023 20:36:50 +0800

> On 2023/7/27 22:43, Alexander Lobakin wrote:
> 
>>  
>>  struct page_pool {
>>  	struct page_pool_params p;
>> -	long pad;
>> +
>> +	bool dma_map:1;				/* Perform DMA mapping */
>> +	enum {
>> +		PP_DMA_SYNC_ACT_DISABLED = 0,	/* Driver didn't ask to sync */
>> +		PP_DMA_SYNC_ACT_DO,		/* Perform DMA sync ops */
>> +	} dma_sync_act:1;
>> +	bool page_frag:1;			/* Allow page fragments */
>>  
> 
> Isn't it more common or better to just remove the flags field in
> 'struct page_pool_params' and pass the flags by parameter like
> below, so that patch 4 is not needed?
> 
> struct page_pool *page_pool_create(const struct page_pool_params *params,
> 				   unsigned int	flags);

You would need a separate patch to convert all the page_pool_create()
users then either way.
And it doesn't look really natural to me to pass both driver-set params
and driver-set flags as separate function arguments. Someone may then
think "why aren't flags just put in the params itself". The fact that
Page Pool copies the whole params in the page_pool struct after
allocating it is internals, page_pool_create() prototype however isn't.
Thoughts?

Thanks,
Olek