linux-kernel - Re: [PATCH v2 0/5] drm/amd/display: Stop control flow if the divisior is zero

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <97d1f078-981d-ed10-7e6f-a9e177298b2e@loongson.cn>
Date: Sat, 11 Jan 2025 15:03:09 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: Harry Wentland <harry.wentland@....com>,
 Alex Deucher <alexander.deucher@....com>,
 Christian König <christian.koenig@....com>,
 Xinhui Pan <Xinhui.Pan@....com>, Leo Li <sunpeng.li@....com>,
 Rodrigo Siqueira <Rodrigo.Siqueira@....com>
Cc: Nathan Chancellor <nathan@...nel.org>,
 Josh Poimboeuf <jpoimboe@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
 loongarch@...ts.linux.dev, amd-gfx@...ts.freedesktop.org,
 llvm@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/5] drm/amd/display: Stop control flow if the divisior
 is zero

On 01/11/2025 05:45 AM, Harry Wentland wrote:
> On 2025-01-06 03:57, Tiezhu Yang wrote:
>> As far as I can tell, with the current existing macro definitions, there
>> is no better way to do the minimal and proper changes to stop the control
>> flow if the divisior is zero.
>>
>> In order to keep the current ability for the aim of debugging and avoid
>> printing the warning message twice, it is better to only use ASSERT_BUG()
>> and SPL_ASSERT_BUG() directly after doing the following four steps:
>>
>> (1) Replace ASSERT() with ASSERT_WARN()
>> (2) Replace SPL_ASSERT() with SPL_ASSERT_WARN()
>> (3) Add ASSERT_BUG() macro definition
>> (4) Add SPL_ASSERT_BUG() macro definition
>>
>
> Patches 1-4 create lots of churn across the whole driver for little
> immediate benefit. We should be able to do patch 5 without requiring
> all that churn.

Do you mean to drop the first 4 patches and only do the following
simple changes to replace the current ASSERT() and SPL_ASSERT() with
BUG_ON() directly without considering the aim of debugging? Something
like this:

--- >8 ---

diff --git a/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c 
b/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
index 88d3f9d7dd55..ec6b194fb093 100644
--- a/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
+++ b/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
@@ -51,7 +51,7 @@ static inline unsigned long long 
complete_integer_division_u64(
  {
         unsigned long long result;

-       ASSERT(divisor);
+       BUG_ON(divisor);

         result = div64_u64_rem(dividend, divisor, remainder);

diff --git a/drivers/gpu/drm/amd/display/dc/spl/spl_fixpt31_32.c 
b/drivers/gpu/drm/amd/display/dc/spl/spl_fixpt31_32.c
index 131f1e3949d3..a91dbd076d13 100644
--- a/drivers/gpu/drm/amd/display/dc/spl/spl_fixpt31_32.c
+++ b/drivers/gpu/drm/amd/display/dc/spl/spl_fixpt31_32.c
@@ -29,7 +29,7 @@ static inline unsigned long long 
spl_complete_integer_division_u64(
  {
         unsigned long long result;

-       SPL_ASSERT(divisor);
+       BUG_ON(divisor);

         result = spl_div64_u64_rem(dividend, divisor, remainder);

or keep the ASSERT() and SPL_ASSERT() definitions as is, just add
ASSERT_BUG() and SPL_ASSERT_BUG() definitions, then replace the
current ASSERT() and SPL_ASSERT() for the two places, that is to
say, drop the first 2 patches and squash the last 3 patches to one
single patch?

Thanks,
Tiezhu