From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Mavrodiev Date: Thu, 20 Dec 2018 14:46:01 +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> <77fb61ff-ff93-586a-4231-d3f1dca0d54a@olimex.com> Message-ID: <0ea88b7d-e74e-12f9-0b2f-1f108f602d7f@olimex.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 12/20/18 2:44 PM, Jagan Teki wrote: > On Thu, Dec 20, 2018 at 6:12 PM Stefan Mavrodiev wrote: >> >> On 12/20/18 2:38 PM, Jagan Teki wrote: >>> On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev wrote: >>>> 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: >>> OK, thanks for the details and test. >>> >>> Please send the v2 with proper commit head it should be "spi: sun4i: ..." >>> Also I didn't find this patch on patchwork, don't know why? >> No idea. I can see it here: >> https://patchwork.ozlabs.org/patch/1008181/ > OK, got it. I will format the commit and apply if are OK? Sure. Thank you.