| Message ID | 20240705134221.2f4de205caa1.I28496dc0f2ced580282d1fb892048017c4491e21@changeid (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: page_pool: fix warning code | expand |
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
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
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
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
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 >
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
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
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
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
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
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
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
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);