| Message ID | 20200821161401.11307-7-l.stelmach@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some fixes for spi-s3c64xx | expand |
On Fri, Aug 21, 2020 at 06:13:58PM +0200, Łukasz Stelmach wrote: > Check return values in prepare_dma() and s3c64xx_spi_config() and > propagate errors upwards. > > Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> > --- > drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- > 1 file changed, 39 insertions(+), 8 deletions(-) This should be a third patch - backportable fixes should go at beginning. Fixes: 788437273fa8 ("spi: s3c64xx: move to generic dmaengine API") Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On 8/21/20 18:13, Łukasz Stelmach wrote: > Check return values in prepare_dma() and s3c64xx_spi_config() and > propagate errors upwards. > > Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com> > --- > drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- > 1 file changed, 39 insertions(+), 8 deletions(-) > @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, > > desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, > dma->direction, DMA_PREP_INTERRUPT); > + if (!desc) { > + dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist", > + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx"); > + return -ENOMEM; > + } > > desc->callback = s3c64xx_spi_dmacb; > desc->callback_param = dma; > > dma->cookie = dmaengine_submit(desc); > + ret = dma_submit_error(dma->cookie); > + if (ret) { > + dev_err(&sdd->pdev->dev, "DMA submission failed"); > + return -EIO; Just return the error value from dma_submit_error() here?
It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote: > On 8/21/20 18:13, Łukasz Stelmach wrote: >> Check return values in prepare_dma() and s3c64xx_spi_config() and >> propagate errors upwards. >> >> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com> >> --- >> drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- >> 1 file changed, 39 insertions(+), 8 deletions(-) > >> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, >> desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, >> dma->direction, DMA_PREP_INTERRUPT); >> + if (!desc) { >> + dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist", >> + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx"); >> + return -ENOMEM; >> + } >> desc->callback = s3c64xx_spi_dmacb; >> desc->callback_param = dma; >> dma->cookie = dmaengine_submit(desc); >> + ret = dma_submit_error(dma->cookie); >> + if (ret) { >> + dev_err(&sdd->pdev->dev, "DMA submission failed"); >> + return -EIO; > > Just return the error value from dma_submit_error() here? > --8<---------------cut here---------------start------------->8--- static inline int dma_submit_error(dma_cookie_t cookie) { return cookie < 0 ? cookie : 0; } --8<---------------cut here---------------end--------------->8--- Not quite meaningful IMHO, is it?
On 9/1/20 17:21, Lukasz Stelmach wrote: > It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote: >> On 8/21/20 18:13, Łukasz Stelmach wrote: >>> Check return values in prepare_dma() and s3c64xx_spi_config() and >>> propagate errors upwards. >>> >>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com> >>> --- >>> drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- >>> 1 file changed, 39 insertions(+), 8 deletions(-) >> >>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, >>> desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, >>> dma->direction, DMA_PREP_INTERRUPT); >>> + if (!desc) { >>> + dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist", >>> + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx"); >>> + return -ENOMEM; >>> + } >>> desc->callback = s3c64xx_spi_dmacb; >>> desc->callback_param = dma; >>> dma->cookie = dmaengine_submit(desc); >>> + ret = dma_submit_error(dma->cookie); >>> + if (ret) { >>> + dev_err(&sdd->pdev->dev, "DMA submission failed"); >>> + return -EIO; >> >> Just return the error value from dma_submit_error() here? >> > > --8<---------------cut here---------------start------------->8--- > static inline int dma_submit_error(dma_cookie_t cookie) > { > return cookie < 0 ? cookie : 0; > > } > --8<---------------cut here---------------end--------------->8--- > > Not quite meaningful IMHO, is it? dma_submit_error() returns 0 or an error code, I think it makes sense to propagate that error code rather than replacing it with -EIO.
It was <2020-09-02 śro 10:14>, when Sylwester Nawrocki wrote: > On 9/1/20 17:21, Lukasz Stelmach wrote: >> It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote: >>> On 8/21/20 18:13, Łukasz Stelmach wrote: >>>> Check return values in prepare_dma() and s3c64xx_spi_config() and >>>> propagate errors upwards. >>>> >>>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com> >>>> --- >>>> drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- >>>> 1 file changed, 39 insertions(+), 8 deletions(-) >>> >>>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, >>>> desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, >>>> dma->direction, DMA_PREP_INTERRUPT); >>>> + if (!desc) { >>>> + dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist", >>>> + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx"); >>>> + return -ENOMEM; >>>> + } >>>> desc->callback = s3c64xx_spi_dmacb; >>>> desc->callback_param = dma; >>>> dma->cookie = dmaengine_submit(desc); >>>> + ret = dma_submit_error(dma->cookie); >>>> + if (ret) { >>>> + dev_err(&sdd->pdev->dev, "DMA submission failed"); >>>> + return -EIO; >>> >>> Just return the error value from dma_submit_error() here? >>> >> >> --8<---------------cut here---------------start------------->8--- >> static inline int dma_submit_error(dma_cookie_t cookie) >> { >> return cookie < 0 ? cookie : 0; >> >> } >> --8<---------------cut here---------------end--------------->8--- >> >> Not quite meaningful IMHO, is it? > > dma_submit_error() returns 0 or an error code, I think it makes sense > to propagate that error code rather than replacing it with -EIO. It is not an error code that d_s_e() returns it is a value returned by dma_cookie_assigned() called from within the tx_submit() operaton of a DMA driver. --8<---------------cut here---------------start------------->8--- static inline dma_cookie_t dma_cookie_assign(struct dma_async_tx_descriptor *tx) { struct dma_chan *chan = tx->chan; dma_cookie_t cookie; cookie = chan->cookie + 1; if (cookie < DMA_MIN_COOKIE) cookie = DMA_MIN_COOKIE; tx->cookie = chan->cookie = cookie; return cookie; } --8<---------------cut here---------------end--------------->8--- Yes, a non-zero value returned by d_s_e() indicates an error but it definitely isn't one of error codes from errno*.h.
On 9/3/20 10:45, Lukasz Stelmach wrote: > It was <2020-09-02 śro 10:14>, when Sylwester Nawrocki wrote: >> On 9/1/20 17:21, Lukasz Stelmach wrote: >>> It was <2020-08-25 wto 21:06>, when Sylwester Nawrocki wrote: >>>> On 8/21/20 18:13, Łukasz Stelmach wrote: >>>>> Check return values in prepare_dma() and s3c64xx_spi_config() and >>>>> propagate errors upwards. >>>>> >>>>> Signed-off-by: Łukasz Stelmach<l.stelmach@samsung.com> >>>>> --- >>>>> drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- >>>>> 1 file changed, 39 insertions(+), 8 deletions(-) >>>> >>>>> @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, >>>>> desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, >>>>> dma->direction, DMA_PREP_INTERRUPT); >>>>> + if (!desc) { >>>>> + dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist", >>>>> + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx"); >>>>> + return -ENOMEM; >>>>> + } >>>>> desc->callback = s3c64xx_spi_dmacb; >>>>> desc->callback_param = dma; >>>>> dma->cookie = dmaengine_submit(desc); >>>>> + ret = dma_submit_error(dma->cookie); >>>>> + if (ret) { >>>>> + dev_err(&sdd->pdev->dev, "DMA submission failed"); >>>>> + return -EIO; >>>> >>>> Just return the error value from dma_submit_error() here? >>>> >>> >>> --8<---------------cut here---------------start------------->8--- >>> static inline int dma_submit_error(dma_cookie_t cookie) >>> { >>> return cookie < 0 ? cookie : 0; >>> >>> } >>> --8<---------------cut here---------------end--------------->8--- >>> >>> Not quite meaningful IMHO, is it? >> >> dma_submit_error() returns 0 or an error code, I think it makes sense >> to propagate that error code rather than replacing it with -EIO. > > It is not an error code that d_s_e() returns it is a value returned by > dma_cookie_assign() called from within the tx_submit() operation of a > DMA driver. > > --8<---------------cut here---------------start------------->8--- > static inline dma_cookie_t dma_cookie_assign(struct > dma_async_tx_descriptor *tx) > { > struct dma_chan *chan = tx->chan; > dma_cookie_t cookie; > > cookie = chan->cookie + 1; > if (cookie < DMA_MIN_COOKIE) > cookie = DMA_MIN_COOKIE; > tx->cookie = chan->cookie = cookie; > > return cookie; > } > --8<---------------cut here---------------end--------------->8--- > > Yes, a non-zero value returned by d_s_e() indicates an error but it > definitely isn't one of error codes from errno*.h. I guess we can end that discussion at this point and keep your patch as is, I just followed comment at the dma_submit_error() function: "if dma_cookie_t is >0 it's a DMA request cookie, <0 it's an error code" AFAICS dma_cookie_assign() always returns value > 0 and dma_submit_error() only returns the cookie if its value is < 0 so in consequence d_s_e() will be always returning 0 in your case (PL330 DMAC)? The below commit, likely a result of static code analysis, might be a confirmation. It could also explain why some drivers overwrite the return value of d_s_e() and some just pass it up to the callers. --------------------------------8<------------------------------------ commit 71ea148370f8b6c745a8a42f6fd983cf5ebade18 Author: Dan Carpenter <dan.carpenter@oracle.com> Date: Sat Aug 10 10:46:50 2013 +0300 dmaengine: make dma_submit_error() return an error code The problem here is that the dma_xfer() functions in drivers/ata/pata_arasan_cf.c and drivers/mtd/nand/fsmc_nand.c expect dma_submit_error() to return an error code so they return 1 when they intended to return a negative. So far as I can tell, none of the ->tx_submit() functions ever do return error codes so this patch should have no effect in the current code. I also changed it from a define to an inline. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Dan Williams <djbw@fb.com> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index cb286b1a..b3ba7e4 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -38,7 +38,10 @@ typedef s32 dma_cookie_t; #define DMA_MIN_COOKIE 1 #define DMA_MAX_COOKIE INT_MAX -#define dma_submit_error(cookie) ((cookie) < 0 ? 1 : 0) +static inline int dma_submit_error(dma_cookie_t cookie) +{ + return cookie < 0 ? cookie : 0; +} --------------------------------8<------------------------------------
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c index 6381a7557def..02de734b8ab1 100644 --- a/drivers/spi/spi-s3c64xx.c +++ b/drivers/spi/spi-s3c64xx.c @@ -269,12 +269,13 @@ static void s3c64xx_spi_dmacb(void *data) spin_unlock_irqrestore(&sdd->lock, flags); } -static void prepare_dma(struct s3c64xx_spi_dma_data *dma, +static int prepare_dma(struct s3c64xx_spi_dma_data *dma, struct sg_table *sgt) { struct s3c64xx_spi_driver_data *sdd; struct dma_slave_config config; struct dma_async_tx_descriptor *desc; + int ret; memset(&config, 0, sizeof(config)); @@ -298,12 +299,24 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma, desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents, dma->direction, DMA_PREP_INTERRUPT); + if (!desc) { + dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist", + dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx"); + return -ENOMEM; + } desc->callback = s3c64xx_spi_dmacb; desc->callback_param = dma; dma->cookie = dmaengine_submit(desc); + ret = dma_submit_error(dma->cookie); + if (ret) { + dev_err(&sdd->pdev->dev, "DMA submission failed"); + return -EIO; + } + dma_async_issue_pending(dma->ch); + return 0; } static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable) @@ -353,11 +366,12 @@ static bool s3c64xx_spi_can_dma(struct spi_master *master, return xfer->len > (FIFO_LVL_MASK(sdd) >> 1) + 1; } -static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, +static int s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, struct spi_transfer *xfer, int dma_mode) { void __iomem *regs = sdd->regs; u32 modecfg, chcfg; + int ret = 0; modecfg = readl(regs + S3C64XX_SPI_MODE_CFG); modecfg &= ~(S3C64XX_SPI_MODE_TXDMA_ON | S3C64XX_SPI_MODE_RXDMA_ON); @@ -383,7 +397,7 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, chcfg |= S3C64XX_SPI_CH_TXCH_ON; if (dma_mode) { modecfg |= S3C64XX_SPI_MODE_TXDMA_ON; - prepare_dma(&sdd->tx_dma, &xfer->tx_sg); + ret = prepare_dma(&sdd->tx_dma, &xfer->tx_sg); } else { switch (sdd->cur_bpw) { case 32: @@ -415,12 +429,17 @@ static void s3c64xx_enable_datapath(struct s3c64xx_spi_driver_data *sdd, writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff) | S3C64XX_SPI_PACKET_CNT_EN, regs + S3C64XX_SPI_PACKET_CNT); - prepare_dma(&sdd->rx_dma, &xfer->rx_sg); + ret = prepare_dma(&sdd->rx_dma, &xfer->rx_sg); } } + if (ret) + return ret; + writel(modecfg, regs + S3C64XX_SPI_MODE_CFG); writel(chcfg, regs + S3C64XX_SPI_CH_CFG); + + return 0; } static u32 s3c64xx_spi_wait_for_timeout(struct s3c64xx_spi_driver_data *sdd, @@ -553,9 +572,10 @@ static int s3c64xx_wait_for_pio(struct s3c64xx_spi_driver_data *sdd, return 0; } -static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) +static int s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) { void __iomem *regs = sdd->regs; + int ret; u32 val; /* Disable Clock */ @@ -603,7 +623,9 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) if (sdd->port_conf->clk_from_cmu) { /* The src_clk clock is divided internally by 2 */ - clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); + ret = clk_set_rate(sdd->src_clk, sdd->cur_speed * 2); + if (ret) + return ret; } else { /* Configure Clock */ val = readl(regs + S3C64XX_SPI_CLK_CFG); @@ -617,6 +639,8 @@ static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd) val |= S3C64XX_SPI_ENCLK_ENABLE; writel(val, regs + S3C64XX_SPI_CLK_CFG); } + + return 0; } #define XFER_DMAADDR_INVALID DMA_BIT_MASK(32) @@ -659,7 +683,9 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master, sdd->cur_bpw = bpw; sdd->cur_speed = speed; sdd->cur_mode = spi->mode; - s3c64xx_spi_config(sdd); + status = s3c64xx_spi_config(sdd); + if (status) + return status; } if (!is_polling(sdd) && (xfer->len > fifo_len) && @@ -686,10 +712,15 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master, /* Start the signals */ s3c64xx_spi_set_cs(spi, true); - s3c64xx_enable_datapath(sdd, xfer, use_dma); + status = s3c64xx_enable_datapath(sdd, xfer, use_dma); spin_unlock_irqrestore(&sdd->lock, flags); + if (status) { + dev_err(&spi->dev, "failed to enable data path for transfer: %d\n", status); + break; + } + if (use_dma) status = s3c64xx_wait_for_dma(sdd, xfer); else
Check return values in prepare_dma() and s3c64xx_spi_config() and propagate errors upwards. Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com> --- drivers/spi/spi-s3c64xx.c | 47 ++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 8 deletions(-)