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: <8876844c-8126-485d-8e5a-f984bda7241d@amd.com>
Date: Mon, 13 Jan 2025 10:28:52 -0500
From: Harry Wentland <harry.wentland@....com>
To: Tiezhu Yang <yangtiezhu@...ngson.cn>,
 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 2025-01-11 02:03, Tiezhu Yang wrote:
> 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?


I think the second one is better, so drop 1-2 and keep 3-5, basically.
It's fine to keep 3-5 as separate patches or squash. I'm happy either
way.

Thanks,
Harry

> 
> Thanks,
> Tiezhu
>