Re: [PATCH v6 06/13] drm/bridge: it6505: avoid division by zero in pixel clock calculation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unhandled hardware state when reading REG_AUDIO_INPUT_FREQ leads to division by zero and kernel panic.
- [Medium] Missing error handling for I2C reads leads to data corruption in video timing calculations.
- [Medium] Data race in `it6505_calc_video_info` corrupts pixel clock counter reading.
--

--- Patch [6]: [PATCH v6 06/13] drm/bridge: it6505: avoid division by zero in pixel clock calculation ---
commit 00522edef86150c061773cb0103cee92136a7a79
Author: Daniel Golle <daniel@makrotopia.org>
Subject: drm/bridge: it6505: avoid division by zero in pixel clock calculation

This commit fixes a potential division by zero in the pixel clock
computation. It moves the division of the counter sum to after the zero
check to avoid truncation of small values. This prevents sums of 1 or 2
from being evaluated as 0 and bypassing the check.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index 3806b0b966376..b7eb746c8f8e5 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> @@ -751,12 +751,13 @@ static void it6505_calc_video_info(struct it6505 *it6505)
>  		sum += rddata;

[Severity: Medium]
This is a pre-existing issue, but does this bitwise mask without checking for
negative error codes cause data corruption?

If I2C reads fail and return -ENODEV, the negative return value is
directly bitwise-masked and treated as a valid hardware reading.

For example, earlier in it6505_calc_video_info():

    htotal = it6505_read_word(it6505, REG_INPUT_HTOTAL) & 0x1FFF;

And similarly for the counter:

    rddata = it6505_read_word(it6505, REG_PCLK_COUNTER_VALUE) & 0xFFF;
    sum += rddata;

Could this lead to negative error codes being added to the counter sum,
corrupting the subsequent pixel clock math?

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a data race in
it6505_calc_video_info() that corrupts the pixel clock counter reading?

This function performs a multi-step hardware sequence without holding any
locks:

    it6505_set_bits(it6505, REG_DATA_CTRL0, ENABLE_PCLK_COUNTER,
                    ENABLE_PCLK_COUNTER);
    usleep_range(10000, 15000);
    it6505_set_bits(it6505, REG_DATA_CTRL0, ENABLE_PCLK_COUNTER, 0x00);
    rddata = ...

Since this can be executed concurrently by the threaded IRQ handler
it6505_irq_scdt() and the debugfs read handler
receive_timing_debugfs_show(), could a debugfs read occurring during the IRQ
handling interleave register writes?

One thread might disable the counter prematurely while the other is sleeping,
resulting in an abnormally small reading.

Is this race condition the root cause of the division by zero that this patch
attempts to mitigate?

>  	}
>  
> +	sum /= 3;
> +
>  	if (sum == 0) {
>  		DRM_DEV_DEBUG_DRIVER(dev, "calc video timing error");
>  		return;
>  	}
>  
> -	sum /= 3;
>  	pclk = 13500 * 2048 / sum;
>  	it6505->video_info.clock = pclk;
>  	it6505->video_info.hdisplay = hdew;

[Severity: High]
This is a pre-existing issue not introduced by this patch, but can reading an
unhandled hardware state lead to a kernel panic?

Looking at it6505_enable_audio():

    regbe = it6505_read(it6505, REG_AUDIO_INPUT_FREQ);
    DRM_DEV_DEBUG_DRIVER(dev, "regbe:0x%02x audio input fs: %d.%d kHz",
                         regbe, 6750 / regbe, (6750 % regbe) * 10 / regbe);

If the hardware register contains 0, for instance if the audio clock is not
fully active or uninitialized, will the subsequent DRM_DEV_DEBUG_DRIVER macro
trigger a division by zero when evaluating 6750 / regbe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784947240.git.daniel@makrotopia.org?part=6