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.