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
>
