From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Mavrodiev <stefan@olimex.com>
Date: Thu, 20 Dec 2018 08:45:09 +0200
Subject: [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check
In-Reply-To: <39708f5a-fa98-6407-e038-e3d5df958c41@olimex.com>
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>
 <39708f5a-fa98-6407-e038-e3d5df958c41@olimex.com>
Message-ID: <8795a0ed-6472-8ae5-f98a-1c751305f1a4@olimex.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de


On 12/17/18 8:37 AM, Stefan Mavrodiev wrote:
>
> 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 <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'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
>>

Any comments?
I think with all said, the patch can be merged as it is.