Re: [PATCH v5 02/11] drm/panfrost: Prevent division by 0

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Multiplying `status->busy_time` by 100 introduces a 32-bit integer overflow on 32-bit architectures, resulting in wildly incorrect GPU load percentages in the debug log.
- [Low] The use of the type-unsafe `MAX()` macro violates kernel coding style guidelines.
--

--- Patch [2]: [PATCH v5 02/11] drm/panfrost: Prevent division by 0 ---
commit 40a64804964495fc5a5f268bdf1f61ec7215f8a4
Author: Adrián Larumbe <[email protected]>

drm/panfrost: Prevent division by 0

This commit attempts to fix a potential division by zero when calculating the
GPU load percentage for debug logging by rearranging the operands. It changes
the calculation to multiply busy_time by 100 before dividing, and uses MAX()
to ensure the divisor is at least 1.

Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88

> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -78,7 +78,7 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
>  
>  	dev_dbg(pfdev->base.dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
>  		status->busy_time, status->total_time,
> -		status->busy_time / (status->total_time / 100),
> +		status->busy_time * 100 / MAX(status->total_time, 1),

[Severity: Medium]
Does this calculation introduce a 32-bit integer overflow on 32-bit platforms
like ARMv7?

The fields busy_time and total_time in struct devfreq_dev_status are of type
unsigned long. On 32-bit architectures, unsigned long is 32 bits, with a
maximum value of 4,294,967,295.

Since time is measured in nanoseconds and the devfreq polling interval is
typically 50ms, a fully loaded GPU will have a busy_time close to 50,000,000 ns.

Multiplying this by 100 yields 5,000,000,000, which overflows the 32-bit
unsigned integer and wraps around to ~705 million. This results in the debug
log incorrectly reporting a ~14% load instead of 100% when the GPU is fully
utilized.

[Severity: Low]
This isn't a bug introduced by this patch, but does the use of MAX() here
bypass strict kernel type-checking mechanisms?

The MAX() macro defined in linux/minmax.h is explicitly documented with a
warning to use it carefully for obvious constants only, as it performs no type
checking and uses the arguments multiple times.

While this specific instance does not cause a runtime bug, using the type-safe
max(status->total_time, 1UL) or max_t(unsigned long, status->total_time, 1)
might be a better fit for standard kernel coding style guidelines.

>  		status->current_frequency / 1000 / 1000);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2