[PATCH net-next] net: page_pool: fix warning code

[PATCH net-next] net: page_pool: fix warning code

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

WARN_ON_ONCE("string") doesn't really do what appears to
be intended, so fix that.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/core/page_pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 3927a0a7fa9a..97914a9e2a85 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -445,7 +445,7 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
 	return true;
 
 unmap_failed:
-	WARN_ON_ONCE("unexpected DMA address, please report to netdev@");
+	WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
 	dma_unmap_page_attrs(pool->p.dev, dma,
 			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
 			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);
-- 
2.45.2

Re: [PATCH net-next] net: page_pool: fix warning code

[Alexander Lobakin]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri,  5 Jul 2024 13:42:06 +0200

> From: Johannes Berg <johannes.berg@intel.com>
> 
> WARN_ON_ONCE("string") doesn't really do what appears to
> be intended, so fix that.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

"Fixes:" tag?

> ---
>  net/core/page_pool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 3927a0a7fa9a..97914a9e2a85 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -445,7 +445,7 @@ static bool page_pool_dma_map(struct page_pool *pool, struct page *page)
>  	return true;
>  
>  unmap_failed:
> -	WARN_ON_ONCE("unexpected DMA address, please report to netdev@");
> +	WARN_ONCE(1, "unexpected DMA address, please report to netdev@");
>  	dma_unmap_page_attrs(pool->p.dev, dma,
>  			     PAGE_SIZE << pool->p.order, pool->p.dma_dir,
>  			     DMA_ATTR_SKIP_CPU_SYNC | DMA_ATTR_WEAK_ORDERING);

Thanks,
Olek

Re: [PATCH net-next] net: page_pool: fix warning code

[Johannes Berg]

On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri,  5 Jul 2024 13:42:06 +0200
> 
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > WARN_ON_ONCE("string") doesn't really do what appears to
> > be intended, so fix that.
> > 
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> "Fixes:" tag?

There keep being discussions around this so I have no idea what's the
guideline-du-jour ... It changes the code but it's not really an issue?

johannes

Re: [PATCH net-next] net: page_pool: fix warning code

[Alexander Lobakin]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 05 Jul 2024 14:33:31 +0200

> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Date: Fri,  5 Jul 2024 13:42:06 +0200
>>
>>> From: Johannes Berg <johannes.berg@intel.com>
>>>
>>> WARN_ON_ONCE("string") doesn't really do what appears to
>>> be intended, so fix that.
>>>
>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>
>> "Fixes:" tag?
> 
> There keep being discussions around this so I have no idea what's the
> guideline-du-jour ... It changes the code but it's not really an issue?

Hmm, it's an incorrect usage of WARN_ON() (a string is passed instead of
a warning condition), so I do believe this should go as a fix.
Maybe let the maintainers respond what they think.

> 
> johannes

Thanks,
Olek

Re: [PATCH net-next] net: page_pool: fix warning code

[Johannes Berg]

On Fri, 2024-07-05 at 14:37 +0200, Alexander Lobakin wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri, 05 Jul 2024 14:33:31 +0200
> 
> > On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
> > > From: Johannes Berg <johannes@sipsolutions.net>
> > > Date: Fri,  5 Jul 2024 13:42:06 +0200
> > > 
> > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > 
> > > > WARN_ON_ONCE("string") doesn't really do what appears to
> > > > be intended, so fix that.
> > > > 
> > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > "Fixes:" tag?
> > 
> > There keep being discussions around this so I have no idea what's the
> > guideline-du-jour ... It changes the code but it's not really an issue?
> 
> Hmm, it's an incorrect usage of WARN_ON() (a string is passed instead of
> a warning condition),

Well, yes, but the intent was clearly to unconditionally trigger a
warning with a message, and the only thing getting lost is the message;
if you look up the warning in the code you still see it. But anyway, I
don't care.

The tag would be

Fixes: 90de47f020db ("page_pool: fragment API support for 32-bit arch with 64-bit DMA")

if anyone wants it :)

johannes

Re: [PATCH net-next] net: page_pool: fix warning code

[Przemek Kitszel]

On 7/5/24 14:39, Johannes Berg wrote:
> On Fri, 2024-07-05 at 14:37 +0200, Alexander Lobakin wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Date: Fri, 05 Jul 2024 14:33:31 +0200
>>
>>> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
>>>> From: Johannes Berg <johannes@sipsolutions.net>
>>>> Date: Fri,  5 Jul 2024 13:42:06 +0200
>>>>
>>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>>
>>>>> WARN_ON_ONCE("string") doesn't really do what appears to
>>>>> be intended, so fix that.
>>>>>
>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>
>>>> "Fixes:" tag?
>>>
>>> There keep being discussions around this so I have no idea what's the
>>> guideline-du-jour ... It changes the code but it's not really an issue?
>>
>> Hmm, it's an incorrect usage of WARN_ON() (a string is passed instead of
>> a warning condition),
> 
> Well, yes, but the intent was clearly to unconditionally trigger a
> warning with a message, and the only thing getting lost is the message;
> if you look up the warning in the code you still see it. But anyway, I
> don't care.
> 

for the record, [1] tells: to the -next

[1] 
https://lore.kernel.org/netdev/20240704072155.2ea340a9@kernel.org/T/#m919e75afc977fd250ec8c4fa37a2fb1e5baadd3f

> The tag would be
> 
> Fixes: 90de47f020db ("page_pool: fragment API support for 32-bit arch with 64-bit DMA")
> 
> if anyone wants it :)
> 
> johannes
> 

Re: [PATCH net-next] net: page_pool: fix warning code

[Alexander Lobakin]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Date: Fri, 5 Jul 2024 15:14:53 +0200

> On 7/5/24 14:39, Johannes Berg wrote:
>> On Fri, 2024-07-05 at 14:37 +0200, Alexander Lobakin wrote:
>>> From: Johannes Berg <johannes@sipsolutions.net>
>>> Date: Fri, 05 Jul 2024 14:33:31 +0200
>>>
>>>> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
>>>>> From: Johannes Berg <johannes@sipsolutions.net>
>>>>> Date: Fri,  5 Jul 2024 13:42:06 +0200
>>>>>
>>>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>>>
>>>>>> WARN_ON_ONCE("string") doesn't really do what appears to
>>>>>> be intended, so fix that.
>>>>>>
>>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>>
>>>>> "Fixes:" tag?
>>>>
>>>> There keep being discussions around this so I have no idea what's the
>>>> guideline-du-jour ... It changes the code but it's not really an issue?
>>>
>>> Hmm, it's an incorrect usage of WARN_ON() (a string is passed instead of
>>> a warning condition),
>>
>> Well, yes, but the intent was clearly to unconditionally trigger a
>> warning with a message, and the only thing getting lost is the message;
>> if you look up the warning in the code you still see it. But anyway, I
>> don't care.
>>
> 
> for the record, [1] tells: to the -next
> 
> [1]
> https://lore.kernel.org/netdev/20240704072155.2ea340a9@kernel.org/T/#m919e75afc977fd250ec8c4fa37a2fb1e5baadd3f

But there you have "the exact same binary getting generated", while here
you clearly have functional and binary changes :>

I'd even say it may confuse readers and make someone think that you
should pass a string there, not a condition (weak argument, I know).

> 
>> The tag would be
>>
>> Fixes: 90de47f020db ("page_pool: fragment API support for 32-bit arch
>> with 64-bit DMA")
>>
>> if anyone wants it :)
>>
>> johannes

Thanks,
Olek

Re: [PATCH net-next] net: page_pool: fix warning code

[Przemek Kitszel]

On 7/5/24 15:28, Alexander Lobakin wrote:
> From: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> Date: Fri, 5 Jul 2024 15:14:53 +0200
> 
>> On 7/5/24 14:39, Johannes Berg wrote:
>>> On Fri, 2024-07-05 at 14:37 +0200, Alexander Lobakin wrote:
>>>> From: Johannes Berg <johannes@sipsolutions.net>
>>>> Date: Fri, 05 Jul 2024 14:33:31 +0200
>>>>
>>>>> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
>>>>>> From: Johannes Berg <johannes@sipsolutions.net>
>>>>>> Date: Fri,  5 Jul 2024 13:42:06 +0200
>>>>>>
>>>>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>>>>
>>>>>>> WARN_ON_ONCE("string") doesn't really do what appears to
>>>>>>> be intended, so fix that.
>>>>>>>
>>>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>>>
>>>>>> "Fixes:" tag?
>>>>>
>>>>> There keep being discussions around this so I have no idea what's the
>>>>> guideline-du-jour ... It changes the code but it's not really an issue?
>>>>
>>>> Hmm, it's an incorrect usage of WARN_ON() (a string is passed instead of
>>>> a warning condition),
>>>
>>> Well, yes, but the intent was clearly to unconditionally trigger a
>>> warning with a message, and the only thing getting lost is the message;
>>> if you look up the warning in the code you still see it. But anyway, I
>>> don't care.
>>>
>>
>> for the record, [1] tells: to the -next
>>
>> [1]
>> https://lore.kernel.org/netdev/20240704072155.2ea340a9@kernel.org/T/#m919e75afc977fd250ec8c4fa37a2fb1e5baadd3f
> 
> But there you have "the exact same binary getting generated", while here
> you clearly have functional and binary changes :>

one could say that in this case the change to the binary is meaningless
--
I think that the most important rationale is that this code change would
require a -net level of testing, and in theory changed binary means
"some adjacent bug my now reveal", and for such changes we have -next.

The above even answers why "no binary changes" [1] above should be still
considered -next material: there could be some compiler version/flags
combo that would generate different binary, and we get back to the prev 
paragraph.

> 
> I'd even say it may confuse readers and make someone think that you
> should pass a string there, not a condition (weak argument, I know).
> 
>>
>>> The tag would be
>>>
>>> Fixes: 90de47f020db ("page_pool: fragment API support for 32-bit arch
>>> with 64-bit DMA")
>>>
>>> if anyone wants it :)
>>>
>>> johannes
> 
> Thanks,
> Olek

Re: [PATCH net-next] net: page_pool: fix warning code

[Andrew Lunn]

On Fri, Jul 05, 2024 at 02:33:31PM +0200, Johannes Berg wrote:
> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Fri,  5 Jul 2024 13:42:06 +0200
> > 
> > > From: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > WARN_ON_ONCE("string") doesn't really do what appears to
> > > be intended, so fix that.
> > > 
> > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > 
> > "Fixes:" tag?
> 
> There keep being discussions around this so I have no idea what's the
> guideline-du-jour ... It changes the code but it's not really an issue?

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

> It must either fix a real bug that bothers people or just add a device ID.

It is clearly not the last. So does this actually bother people? I
would say no, the WARN_ON_ONCE() will fire, it will just not be as
helpful as it could be. So for me, this is net-next material and no
need to have it backported to stable.

	Andrew

Re: [PATCH net-next] net: page_pool: fix warning code

[Johannes Berg]

On Fri, 2024-07-05 at 16:19 +0200, Andrew Lunn wrote:
> On Fri, Jul 05, 2024 at 02:33:31PM +0200, Johannes Berg wrote:
> > On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
> > > From: Johannes Berg <johannes@sipsolutions.net>
> > > Date: Fri,  5 Jul 2024 13:42:06 +0200
> > > 
> > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > 
> > > > WARN_ON_ONCE("string") doesn't really do what appears to
> > > > be intended, so fix that.
> > > > 
> > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > 
> > > "Fixes:" tag?
> > 
> > There keep being discussions around this so I have no idea what's the
> > guideline-du-jour ... It changes the code but it's not really an issue?
> 
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

And ... by referring to that you just made a guideline(-du-jour!) that
"Fixes" tag must be accompanied by "Cc: stable@" (because Fixes
_doesn't_ imply stable backport!), and then you apply the stable rules
to that.

The time that any one of us will (have) spent even reading this thread
is clearly longer than it would've taken to add (if desired) or remove
(had I included it and not desired) the tag in the first place ...

johannes

Re: [PATCH net-next] net: page_pool: fix warning code

[Alexander Lobakin]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 05 Jul 2024 16:23:43 +0200

> On Fri, 2024-07-05 at 16:19 +0200, Andrew Lunn wrote:
>> On Fri, Jul 05, 2024 at 02:33:31PM +0200, Johannes Berg wrote:
>>> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
>>>> From: Johannes Berg <johannes@sipsolutions.net>
>>>> Date: Fri,  5 Jul 2024 13:42:06 +0200
>>>>
>>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>>
>>>>> WARN_ON_ONCE("string") doesn't really do what appears to
>>>>> be intended, so fix that.
>>>>>
>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>
>>>> "Fixes:" tag?
>>>
>>> There keep being discussions around this so I have no idea what's the
>>> guideline-du-jour ... It changes the code but it's not really an issue?
>>
>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> 
> And ... by referring to that you just made a guideline(-du-jour!) that
> "Fixes" tag must be accompanied by "Cc: stable@" (because Fixes
> _doesn't_ imply stable backport!), and then you apply the stable rules

The netdev ML is an exception, IIRC we never add "Cc: stable@" and then
after hitting Linus' tree at least some commits with "Fixes:" appears
magically in the stable trees :D

> to that.

Thanks,
Olek

Re: [PATCH net-next] net: page_pool: fix warning code

[Johannes Berg]

On Fri, 2024-07-05 at 16:33 +0200, Alexander Lobakin wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Fri, 05 Jul 2024 16:23:43 +0200
> 
> > On Fri, 2024-07-05 at 16:19 +0200, Andrew Lunn wrote:
> > > On Fri, Jul 05, 2024 at 02:33:31PM +0200, Johannes Berg wrote:
> > > > On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
> > > > > From: Johannes Berg <johannes@sipsolutions.net>
> > > > > Date: Fri,  5 Jul 2024 13:42:06 +0200
> > > > > 
> > > > > > From: Johannes Berg <johannes.berg@intel.com>
> > > > > > 
> > > > > > WARN_ON_ONCE("string") doesn't really do what appears to
> > > > > > be intended, so fix that.
> > > > > > 
> > > > > > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> > > > > 
> > > > > "Fixes:" tag?
> > > > 
> > > > There keep being discussions around this so I have no idea what's the
> > > > guideline-du-jour ... It changes the code but it's not really an issue?
> > > 
> > > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> > 
> > And ... by referring to that you just made a guideline(-du-jour!) that
> > "Fixes" tag must be accompanied by "Cc: stable@" (because Fixes
> > _doesn't_ imply stable backport!), and then you apply the stable rules
> 
> The netdev ML is an exception, IIRC we never add "Cc: stable@" and then
> after hitting Linus' tree at least some commits with "Fixes:" appears
> magically in the stable trees :D

See, all the policy changes are confusing people, but not adding Cc:
stable@ for netdev is not true! :)

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#stable-tree

johannes

Re: [PATCH net-next] net: page_pool: fix warning code

[Alexander Lobakin]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 05 Jul 2024 16:36:28 +0200

> On Fri, 2024-07-05 at 16:33 +0200, Alexander Lobakin wrote:
>> From: Johannes Berg <johannes@sipsolutions.net>
>> Date: Fri, 05 Jul 2024 16:23:43 +0200
>>
>>> On Fri, 2024-07-05 at 16:19 +0200, Andrew Lunn wrote:
>>>> On Fri, Jul 05, 2024 at 02:33:31PM +0200, Johannes Berg wrote:
>>>>> On Fri, 2024-07-05 at 14:32 +0200, Alexander Lobakin wrote:
>>>>>> From: Johannes Berg <johannes@sipsolutions.net>
>>>>>> Date: Fri,  5 Jul 2024 13:42:06 +0200
>>>>>>
>>>>>>> From: Johannes Berg <johannes.berg@intel.com>
>>>>>>>
>>>>>>> WARN_ON_ONCE("string") doesn't really do what appears to
>>>>>>> be intended, so fix that.
>>>>>>>
>>>>>>> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
>>>>>>
>>>>>> "Fixes:" tag?
>>>>>
>>>>> There keep being discussions around this so I have no idea what's the
>>>>> guideline-du-jour ... It changes the code but it's not really an issue?
>>>>
>>>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>>
>>> And ... by referring to that you just made a guideline(-du-jour!) that
>>> "Fixes" tag must be accompanied by "Cc: stable@" (because Fixes
>>> _doesn't_ imply stable backport!), and then you apply the stable rules
>>
>> The netdev ML is an exception, IIRC we never add "Cc: stable@" and then
>> after hitting Linus' tree at least some commits with "Fixes:" appears
>> magically in the stable trees :D
> 
> See, all the policy changes are confusing people, but not adding Cc:
> stable@ for netdev is not true! :)
> 
> https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#stable-tree

Aaaah, this was changed recently, right. Sorry for confusion :z

> 
> johannes

Thanks,
Olek

Re: [PATCH net-next] net: page_pool: fix warning code

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri,  5 Jul 2024 13:42:06 +0200 you wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> WARN_ON_ONCE("string") doesn't really do what appears to
> be intended, so fix that.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> [...]

Here is the summary with links:
  - [net-next] net: page_pool: fix warning code
    https://git.kernel.org/netdev/net-next/c/946b6c48cca4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html