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

[Tiezhu Yang]

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