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
