From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Mavrodiev Date: Mon, 17 Dec 2018 08:37:30 +0200 Subject: [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check In-Reply-To: <20181214155527.pzt4xjrurm3cw5ud@flea> References: <20181205122757.14523-1-stefan@olimex.com> <20181205154619.ffzfl7yt6vfo3qra@flea> <9458ed1f-38de-6be0-8e27-a2d6801492a2@gmail.com> <449750de-5be4-aeb1-9d5c-43b42f175ce0@olimex.com> <20181214092522.3ev2b4d4q33k6zip@flea> <0e57b561-d56c-440a-74f8-6c35236c40dc@olimex.com> <20181214155527.pzt4xjrurm3cw5ud@flea> Message-ID: <39708f5a-fa98-6407-e038-e3d5df958c41@olimex.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On 12/14/18 5:55 PM, Maxime Ripard wrote: > On Fri, Dec 14, 2018 at 04:14:31PM +0200, Stefan Mavrodiev wrote: >> On 12/14/18 11:25 AM, Maxime Ripard wrote: >>> On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote: >>>> On 12/6/18 8:41 AM, Stefan Mavrodiev wrote: >>>>> On 12/5/18 5:46 PM, Maxime Ripard wrote: >>>>>> 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 >>>>>>> --- >>>>>>>   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'm not sure if that can even happen? >>>>> I've tried to check that before draining the receive fifo, but >>>>> then the controller doesn't work. I'm thinking that the fifo must >>>>> be drained in any case. >>>> Any further comments? >>> I was expecting you to comment on whether the FIFO needed to be >>> drained or not :) >> Sorry. I didn't understand that. >> >> Anyway. After some code checking, I found that the FIFO needs to be drained >> because TP_EN (Transmit Pause Enable) bit is set during bus claim. >> >> ".... >> >> In master mode, it is used to control transmit state machine to >> stop smart burst sending when RX FIFO is full. >> >> ..." >> >> Perhaps this bit should be enabled only when we want to read back data? > It's been a while since I last looked at the spi driver. What is linux > doing? In the kernel version there is the same check, like the one in the patch: drivers/spi/spi-sun4i.c: static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len) ...... while (len--) {     byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);     if (sspi->rx_buf)         *sspi->rx_buf++ = byte; } ..... Guess I've missed this check when I was adopting the driver for u-boot. > > Maxime >