Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i0ad843c9:Fastmail
Subject: Re: [PATCH] drm/sun4i: dsi: Prevent underflow when computing packet
 sizes
To:     =?UTF-8?Q?Jernej_=c5=a0krabec?= <jernej.skrabec@gmail.com>,
        Chen-Yu Tsai <wens@csie.org>,
        Maxime Ripard <mripard@kernel.org>
Cc:     Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-sunxi@lists.linux.dev
References: <20220812031623.34057-1-samuel@sholland.org>
 <8100632.T7Z3S40VBb@jernej-laptop>
From:   Samuel Holland <samuel@sholland.org>
Message-ID: <093fee4d-d0b9-c6f2-8dde-d50b514fbc69@sholland.org>
Date:   Sun, 14 Aug 2022 12:54:39 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <8100632.T7Z3S40VBb@jernej-laptop>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 8/14/22 2:55 AM, Jernej Škrabec wrote:
> Dne petek, 12. avgust 2022 ob 05:16:23 CEST je Samuel Holland napisal(a):
>> Currently, the packet overhead is subtracted using unsigned arithmetic.
>> With a short sync pulse, this could underflow and wrap around to near
>> the maximal u16 value. Fix this by using signed subtraction. The call to
>> max() will correctly handle any negative numbers that are produced.
>>
>> Apply the same fix to the other timings, even though those subtractions
>> are less likely to underflow.
>>
>> Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller
>> support") Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index b4dfa166eccd..34234a144e87
>> 100644
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi
>> *dsi, SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) |
>>  		     SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff));
>>
>>  	regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG,
>>  		     SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE |
>>  		     SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
>>  }
>>
>>  static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>>  				    struct drm_display_mode *mode)
>>  {
>>  	struct mipi_dsi_device *device = dsi->device;
>> -	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
>> +	int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> 
> Nit: mipi_dsi_pixel_format_to_bpp() can return -EINVAL in case of unsupported 
> format. Would it make sense to check it?

The switch statement in mipi_dsi_pixel_format_to_bpp() handles every value in
the enumeration, so I think the -EINVAL is just there to keep GCC from
complaining. If we do want to handle this case, it would need to be in
sun6i_dsi_attach(), since the other places we use mipi_dsi_pixel_format_to_bpp()
are way too late to handle any errors.

Regards,
Samuel