drm/i915: Explicitly cast divisor to fix Coccinelle warning

[PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning

Posted by Thorsten Blum 1 year, 5 months ago

As the comment explains, the if check ensures that the divisor oa_period
is a u32. Explicitly cast oa_period to u32 to remove the following
Coccinelle/coccicheck warning reported by do_div.cocci:

  WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead

Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 0b1cd4c7a525..24722e758aaf 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf *perf,
 			 */
 			if (oa_period <= NSEC_PER_SEC) {
 				u64 tmp = NSEC_PER_SEC;
-				do_div(tmp, oa_period);
+				do_div(tmp, (u32)oa_period);
 				oa_freq_hz = tmp;
 			} else
 				oa_freq_hz = 0;
-- 
2.45.2

Re: [PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning

Posted by Ville Syrjälä 1 year, 5 months ago

On Wed, Jul 10, 2024 at 09:46:51AM +0200, Thorsten Blum wrote:
> As the comment explains, the if check ensures that the divisor oa_period
> is a u32. Explicitly cast oa_period to u32 to remove the following
> Coccinelle/coccicheck warning reported by do_div.cocci:
> 
>   WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> ---
>  drivers/gpu/drm/i915/i915_perf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 0b1cd4c7a525..24722e758aaf 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf *perf,
>  			 */
>  			if (oa_period <= NSEC_PER_SEC) {
>  				u64 tmp = NSEC_PER_SEC;
> -				do_div(tmp, oa_period);
> +				do_div(tmp, (u32)oa_period);

Why is this code even using do_div() when it doesn't need the
remainder?

>  				oa_freq_hz = tmp;
>  			} else
>  				oa_freq_hz = 0;
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning

Posted by Thorsten Blum 1 year, 5 months ago

On 10. Jul 2024, at 13:38, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Jul 10, 2024 at 09:46:51AM +0200, Thorsten Blum wrote:
>> As the comment explains, the if check ensures that the divisor oa_period
>> is a u32. Explicitly cast oa_period to u32 to remove the following
>> Coccinelle/coccicheck warning reported by do_div.cocci:
>> 
>>  WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead
>> 
>> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
>> ---
>> drivers/gpu/drm/i915/i915_perf.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 0b1cd4c7a525..24722e758aaf 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf *perf,
>>  */
>> if (oa_period <= NSEC_PER_SEC) {
>> u64 tmp = NSEC_PER_SEC;
>> - do_div(tmp, oa_period);
>> + do_div(tmp, (u32)oa_period);
> 
> Why is this code even using do_div() when it doesn't need the
> remainder?

do_div() is an optimized 64-by-32 division and the compiler should
automatically remove the remainder if it's not used.

Re: [PATCH] drm/i915: Explicitly cast divisor to fix Coccinelle warning

Posted by Ville Syrjälä 1 year, 5 months ago

On Wed, Jul 10, 2024 at 01:55:32PM +0200, Thorsten Blum wrote:
> On 10. Jul 2024, at 13:38, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Jul 10, 2024 at 09:46:51AM +0200, Thorsten Blum wrote:
> >> As the comment explains, the if check ensures that the divisor oa_period
> >> is a u32. Explicitly cast oa_period to u32 to remove the following
> >> Coccinelle/coccicheck warning reported by do_div.cocci:
> >> 
> >>  WARNING: do_div() does a 64-by-32 division, please consider using div64_u64 instead
> >> 
> >> Signed-off-by: Thorsten Blum <thorsten.blum@toblux.com>
> >> ---
> >> drivers/gpu/drm/i915/i915_perf.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> >> index 0b1cd4c7a525..24722e758aaf 100644
> >> --- a/drivers/gpu/drm/i915/i915_perf.c
> >> +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> @@ -4103,7 +4103,7 @@ static int read_properties_unlocked(struct i915_perf *perf,
> >>  */
> >> if (oa_period <= NSEC_PER_SEC) {
> >> u64 tmp = NSEC_PER_SEC;
> >> - do_div(tmp, oa_period);
> >> + do_div(tmp, (u32)oa_period);
> > 
> > Why is this code even using do_div() when it doesn't need the
> > remainder?
> 
> do_div() is an optimized 64-by-32 division and the compiler should
> automatically remove the remainder if it's not used.

The point is that do_div() is a bad API because it magically
changes the divided in place. There are more sensible 64bit
division helpers in math64.h that can be used instead.

oa_exponent_to_ns() also hand rolls a DIV_ROUND_UP_ULL()
for some reason...

-- 
Ville Syrjälä
Intel