From: Igor Stoppa <igor.stoppa@gmail.com>
To: David Miller <davem@davemloft.net>
Cc: igor.stoppa@huawei.com, huangdaode@hisilicon.com,
yisen.zhuang@huawei.com, salil.mehta@huawei.com,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] ethernet: hnae: drop adhoc assert() macros
Date: Wed, 12 Sep 2018 18:04:30 +0300 [thread overview]
Message-ID: <2f439541-154f-1f17-180e-db0d8f95e010@gmail.com> (raw)
In-Reply-To: <20180911.230716.1048852895308106303.davem@davemloft.net>
On 12/09/18 09:07, David Miller wrote:
> From: Igor Stoppa <igor.stoppa@gmail.com>
> Date: Sat, 8 Sep 2018 18:01:42 +0300
>
>> Replace assert() with a less misleading test_condition() using WARN()
>> Drop one check which had bitrotted and didn't compile anymore.
>>
>> Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
>
> I'm still kind of not happy about this.
>
> Make the driver use kernel interfaces like WARN_ON_ONCE()
> etc. directly instead of defining alias CPP macros private to the
> driver.
>
> If it needs to be conditional upon DEBUG, we have pr_debug() and
> the likes as well.
WARN_ON_ONCE() will display the error message only once, but from its
implementation it looks like it will evaluate the condition anyways,
every time, unless the compiler can do some magic and somehow know that
the condition was true already onece (/include/asm-generic/bug.h:146)
pr_debug() otoh, will print or not print, depending on
CONFIG_DYNAMIC_DEBUG being set or not (/include/linux/printk.h:399)
but the logic we are trying to clean up here seems a bit different:
if (in_debug_mode && error_condition_is_verified)
generate_warning()
Which, I guess, is meant to completely remove *any* test outside of
debug mode.
That is why I used the test_condition() macro, instead of using directly
WARN_ON_ONCE(). Admittedly, it probably would have been better to
depend on CONFIG_DYNAMIC_DEBUG.
Neither using WARN_ON_ONCE(), nor pr_debug() seem to replicate the
initial behavior of the code that is currently in the tree, with assert().
How would you suggest that I keep the same behavior, without the helper
macro? I am probably missing it, but I do not see some facility that
would support the double condition I described above.
--
thanks, igor
prev parent reply other threads:[~2018-09-12 15:04 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-08 15:01 [PATCH v2] ethernet: hnae: drop adhoc assert() macros Igor Stoppa
2018-09-12 6:07 ` David Miller
2018-09-12 15:04 ` Igor Stoppa [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2f439541-154f-1f17-180e-db0d8f95e010@gmail.com \
--to=igor.stoppa@gmail.com \
--cc=davem@davemloft.net \
--cc=huangdaode@hisilicon.com \
--cc=igor.stoppa@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=salil.mehta@huawei.com \
--cc=yisen.zhuang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).