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@xxxxxxxxxxx>
>>>>> ---
>>>>> 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@xxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Dan Williams <djbw@xxxxxx>
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<------------------------------------
