Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] ethernet: hnae: drop adhoc assert() macros
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
References: <20180908150142.27976-1-igor.stoppa@huawei.com>
 <20180911.230716.1048852895308106303.davem@davemloft.net>
From:   Igor Stoppa <igor.stoppa@gmail.com>
Message-ID: <2f439541-154f-1f17-180e-db0d8f95e010@gmail.com>
Date:   Wed, 12 Sep 2018 18:04:30 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180911.230716.1048852895308106303.davem@davemloft.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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