linux-kernel - Re: [PATCH v2 2/2] drm/tiny: ili9486: Do not assume 8-bit only SPI controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <81e5bd05-b081-8072-b09d-30e32834163e@linaro.org>
Date:   Tue, 29 Nov 2022 09:41:11 +0100
From:   Neil Armstrong <neil.armstrong@...aro.org>
To:     Carlo Caione <ccaione@...libre.com>,
        Mark Brown <broonie@...nel.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Kevin Hilman <khilman@...libre.com>,
        Kamlesh Gurudasani <kamlesh.gurudasani@...il.com>,
        Jerome Brunet <jbrunet@...libre.com>
Cc:     dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] drm/tiny: ili9486: Do not assume 8-bit only SPI
 controllers

On 21/11/2022 10:42, Carlo Caione wrote:
> The pixel data for the ILI9486 is always 16-bits wide and it must be
> sent over the SPI bus. When the controller is only able to deal with
> 8-bit transfers, this 16-bits data needs to be swapped before the
> sending to account for the big endian bus, this is on the contrary not
> needed when the SPI controller already supports 16-bits transfers.
> 
> The decision about swapping the pixel data or not is taken in the MIPI
> DBI code by probing the controller capabilities: if the controller only
> suppors 8-bit transfers the data is swapped, otherwise it is not.
> 
> This swapping/non-swapping is relying on the assumption that when the
> controller does support 16-bit transactions then the data is sent
> unswapped in 16-bits-per-word over SPI.
> 
> The problem with the ILI9486 driver is that it is forcing 8-bit
> transactions also for controllers supporting 16-bits, violating the
> assumption and corrupting the pixel data.
> 
> Align the driver to what is done in the MIPI DBI code by adjusting the
> tranfer size to the maximum allowed by the SPI controller.
> 
> Signed-off-by: Carlo Caione <ccaione@...libre.com>
> ---
>   drivers/gpu/drm/tiny/ili9486.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c
> index bd37dfe8dd05..4d80a413338f 100644
> --- a/drivers/gpu/drm/tiny/ili9486.c
> +++ b/drivers/gpu/drm/tiny/ili9486.c
> @@ -43,6 +43,7 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>   			     size_t num)
>   {
>   	struct spi_device *spi = mipi->spi;
> +	unsigned int bpw = 8;
>   	void *data = par;
>   	u32 speed_hz;
>   	int i, ret;
> @@ -56,8 +57,6 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>   	 * The displays are Raspberry Pi HATs and connected to the 8-bit only
>   	 * SPI controller, so 16-bit command and parameters need byte swapping
>   	 * before being transferred as 8-bit on the big endian SPI bus.
> -	 * Pixel data bytes have already been swapped before this function is
> -	 * called.
>   	 */
>   	buf[0] = cpu_to_be16(*cmd);
>   	gpiod_set_value_cansleep(mipi->dc, 0);
> @@ -71,12 +70,18 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>   		for (i = 0; i < num; i++)
>   			buf[i] = cpu_to_be16(par[i]);
>   		num *= 2;
> -		speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
>   		data = buf;
>   	}
>   
> +	/*
> +	 * Check whether pixel data bytes needs to be swapped or not
> +	 */
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +		bpw = 16;
> +
>   	gpiod_set_value_cansleep(mipi->dc, 1);
> -	ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, data, num);
> +	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num);
> +	ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, data, num);
>    free:
>   	kfree(buf);
>   
> Looks fine, but should somehow be tested on an RPi first
to check if the 8bit fallback still works.

Reviewed-by: Neil Armstrong <neil.armstrong@...aro.org>