diff mbox series

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

Message ID 20240705134221.2f4de205caa1.I28496dc0f2ced580282d1fb892048017c4491e21@changeid (mailing list archive)
State Accepted
Commit 946b6c48cca48591fb495508c5dbfade767173d0
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: page_pool: fix warning code | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 816 this patch: 816
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 821 this patch: 821
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 821 this patch: 821
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 3 this patch: 3
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-07-05--15-00 (tests: 694)

Commit Message

Johannes Berg July 5, 2024, 11:42 a.m. UTC
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(-)

Comments

Alexander Lobakin July 5, 2024, 12:32 p.m. UTC | #1
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
Johannes Berg July 5, 2024, 12:33 p.m. UTC | #2
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
Alexander Lobakin July 5, 2024, 12:37 p.m. UTC | #3
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
Johannes Berg July 5, 2024, 12:39 p.m. UTC | #4
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
Przemek Kitszel July 5, 2024, 1:14 p.m. UTC | #5
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
>
Alexander Lobakin July 5, 2024, 1:28 p.m. UTC | #6
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
Przemek Kitszel July 5, 2024, 1:38 p.m. UTC | #7
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
Andrew Lunn July 5, 2024, 2:19 p.m. UTC | #8
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
Johannes Berg July 5, 2024, 2:23 p.m. UTC | #9
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
Alexander Lobakin July 5, 2024, 2:33 p.m. UTC | #10
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
Johannes Berg July 5, 2024, 2:36 p.m. UTC | #11
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
Alexander Lobakin July 5, 2024, 2:44 p.m. UTC | #12
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
patchwork-bot+netdevbpf@kernel.org July 9, 2024, 3:40 a.m. UTC | #13
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!
diff mbox series

Patch

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);