[U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check

[Stefan Mavrodiev]

On 12/20/18 1:54 PM, Jagan Teki wrote:
> On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan at olimex.com> wrote:
>>
>> On 12/20/18 12:56 PM, Jagan Teki wrote:
>>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan at olimex.com> wrote:
>>>> On 12/20/18 12:45 PM, Jagan Teki wrote:
>>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan at olimex.com> wrote:
>>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote:
>>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan at amarulasolutions.com> wrote:
>>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan at olimex.com> 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 at 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;
>>>>>>>> Acked-by: Jagan Teki <jagan at openedev.com>
>>>>>>>>
>>>>>>>> But, have you tested how much data in the fifo before drained? It's
>>>>>>>> better we can get the available data before reading via fifo_sta
>>>>>> I don't understand what's the point of doing this?
>>>>> Didn't get? don't you understand what I'm saying or it not require
>>>>> from point of you?
>>>> Maybe I don't understand correctly what you're saying.
>>> You comment now and previous mail doesn't match. better be specific.
>>>
>>> For draining fifo.
>>> We can find how much data available before reading from fifo and
>>> assign to local rx.
>>>
>>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int len)
>>> {
>>>          u32 reg, cnt
>>>          u8 byte;
>>>
>>>           /* See how much data is available */
>>>           reg = readl(&priv->regs->fifo_sta);
>>>           reg &= SUN4I_FIFO_STA_RF_CNT_MASK;
>>>           cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS;
>>>
>>>           if (len > cnt)
>>>                   len = cnt;
>>>
>>>           while (len--) {
>>>                   byte = readb(&priv->regs->rxdata);
>>>                   *priv->rx_buf++ = byte;
>>>           }
>>> }
>>>
>>> This can be perfect drain fifo, and this is what I'm trying to test
>>> you with existing code and after your patch and verify whether all the
>>> data perfectly drain or not before and after.
>>>
>>> Hope you understand, this time.
>>
>> Hope I understood this time...
>>
>>
>> I've made some modification to sun4i_spi_xfer:
>>
>> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>                 const void *dout, void *din, unsigned long flags)
>> {
>>       struct udevice *bus = dev->parent;
>>       struct sun4i_spi_priv *priv = dev_get_priv(bus);
>>       struct dm_spi_slave_platdata *slave_plat =
>> dev_get_parent_platdata(dev);
>>
>>       u32 len = bitlen / 8;
>>       u32 reg, cnt;
>>       u8 nbytes;
>>       int ret;
>>
>>       priv->tx_buf = dout;
>>       priv->rx_buf = din;
>>
>>       if (bitlen % 8) {
>>           debug("%s: non byte-aligned SPI transfer.\n", __func__);
>>           return -ENAVAIL;
>>       }
>>
>>       if (flags & SPI_XFER_BEGIN)
>>           sun4i_spi_set_cs(bus, slave_plat->cs, true);
>>
>>       reg = readl(&priv->regs->ctl);
>>
>>       /* Reset FIFOs */
>>       writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl);
>>
>>       while (len) {
>>           /* Setup the transfer now... */
>>           nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1));
>>
>>           if (!priv->rx_buf)
>>               printf("%s: Sending %d bytes, ", __func__, nbytes);
>>
>>           /* Setup the counters */
>>           writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc);
>>           writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc);
>>
>>           /* Fill the TX FIFO */
>>           sun4i_spi_fill_fifo(priv, nbytes);
>>
>>           /* Start the transfer */
>>           reg = readl(&priv->regs->ctl);
>>           writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl);
>>
>>           /* Wait transfer to complete */
>>           ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK,
>>                       false, SUN4I_SPI_TIMEOUT_US, false);
>>           if (ret) {
>>               printf("ERROR: sun4i_spi: Timeout transferring data\n");
>>               sun4i_spi_set_cs(bus, slave_plat->cs, false);
>>               return ret;
>>           }
>>
>>           /* Drain the RX FIFO */
>>           if (!priv->rx_buf) {
>>               reg = readl(&priv->regs->fifo_sta);
>>               reg &= SUN4I_FIFO_STA_RF_CNT_MASK;
>>               cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS;
>>               printf("rx fifo: before: %d, ", cnt);
>>           }
>>
>>           sun4i_spi_drain_fifo(priv, nbytes);
> So it's draining fifo without need to checking the available count
> insun4i_spi_drain_fifo(), I mean the below code not require.
>
>          /* See how much data is available */
>          reg = readl(&priv->regs->fifo_sta);
>          reg &= SUN4I_FIFO_STA_RF_CNT_MASK;
>          cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS;
>
>          if (len > cnt)
>                  len = cnt;

Yes, I didn't made any modification to sun4i_spi_drain_fifo() function:

static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int 
len)
{
     u8 byte;

     while (len--) {
         byte = readb(&priv->regs->rxdata);
         if (priv->rx_buf)
             *priv->rx_buf++ = byte;
     }
}