linux-kernel - Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a3e1e8d8-8ad8-6807-6317-caf79f44e91d@kernel.org>
Date:   Thu, 3 Sep 2020 13:18:43 +0200
From:   Sylwester Nawrocki <snawrocki@...nel.org>
To:     Lukasz Stelmach <l.stelmach@...sung.com>
Cc:     Kukjin Kim <kgene@...nel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Andi Shyti <andi@...zian.org>, Mark Brown <broonie@...nel.org>,
        linux-spi@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        m.szyprowski@...sung.com, b.zolnierkie@...sung.com
Subject: Re: [PATCH v2 6/9] spi: spi-s3c64xx: Check return values

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@...sung.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@...cle.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@...cle.com>
    Signed-off-by: Dan Williams <djbw@...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<------------------------------------