From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?UTF-8?Q?Andr=c3=a9_Przywara?= <andre.przywara@arm.com>
Date: Sun, 16 Dec 2018 11:16:00 +0000
Subject: [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check
In-Reply-To: <20181205154619.ffzfl7yt6vfo3qra@flea>
References: <20181205122757.14523-1-stefan@olimex.com>
 <20181205154619.ffzfl7yt6vfo3qra@flea>
Message-ID: <ebc7cfc5-e51b-47e9-14d5-d3fcbf56597d@arm.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 05/12/2018 15:46, Maxime Ripard wrote:

Hi,

> On Wed, Dec 05, 2018 at 02:27:57PM +0200, Stefan Mavrodiev wrote:
>> Current driver doesn't check if the destination pointer is NULL.
>> This cause the data from the FIFO to be stored inside the internal
>> SDRAM ( address 0 ).
>>
>> The patch add simple check if the destination pointer is NULL.
>>
>> Signed-off-by: Stefan Mavrodiev <stefan@olimex.com>
>> ---
>>  drivers/spi/sun4i_spi.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c
>> index b86b5a00ad..38cc743c61 100644
>> --- a/drivers/spi/sun4i_spi.c
>> +++ b/drivers/spi/sun4i_spi.c
>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len)
>>  
>>  	while (len--) {
>>  		byte = readb(&priv->regs->rxdata);
>> -		*priv->rx_buf++ = byte;
>> +		if (priv->rx_buf)
>> +			*priv->rx_buf++ = byte;
> 
> It seems pretty inefficient to test the pointer at each access, it
> would be better to check it once before starting the transfer.

I appreciate the intention to avoid bloat and the attention to detail, but:

This check boils down to exactly one 16-bit instruction:
10c:   b11a	cbz     r2, 116 <sun4i_spi_xfer+0x116>
- which only accesses a register
- inside a loop with does an MMIO(!) read from a device
- handling transfers from a serial device transferring 100s of KB/s
- in a system which's sole purpose is to read data on a single core
- in U-Boot ;-)

So the "performance impact" of this check is probably totally negligible.

There are quite some spi_xfer() calls with explicit NULL arguments, for
instance when we just want to transfer something.

So I wonder if we just want to take this patch, since it fixes a memory
corruption issue, plus we have a similar check in the TX path.

Cheers,
Andre.