From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Mavrodiev Date: Thu, 20 Dec 2018 14:07:50 +0200 Subject: [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check In-Reply-To: References: <20181205122757.14523-1-stefan@olimex.com> <18e25666-911a-8dc6-f8f5-223f1761eafa@olimex.com> <14480b84-da66-885b-38a0-6f414719ee01@olimex.com> Message-ID: <77fb61ff-ff93-586a-4231-d3f1dca0d54a@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/20/18 1:54 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev wrote: >> >> On 12/20/18 12:56 PM, Jagan Teki wrote: >>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev wrote: >>>> On 12/20/18 12:45 PM, Jagan Teki wrote: >>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev wrote: >>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: >>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki wrote: >>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM 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; >>>>>>>> Acked-by: Jagan Teki >>>>>>>> >>>>>>>> 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;     } }