linux-kernel - Re: [PATCH V2] dma: dw-edma-pcie: switch from 'pci_' to 'dma

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 9 Nov 2021 22:11:52 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Qing Wang <wangqing@...o.com>,
        Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Vinod Koul <vkoul@...nel.org>, dmaengine@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Kernel Janitors <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH V2] dma: dw-edma-pcie: switch from 'pci_' to 'dma_' API

Le 09/11/2021 à 14:21, Dan Carpenter a écrit :
> On Tue, Nov 02, 2021 at 08:05:53PM +0100, Christophe JAILLET wrote:
>> Hi,
>>
>>
>> Le 28/09/2021 à 05:44, Qing Wang a écrit :
>>> From: Wang Qing <wangqing@...o.com>
>>>
>>> The wrappers in include/linux/pci-dma-compat.h should go away.
>>>
>>> The patch has been generated with the coccinelle script below.
>>> expression e1, e2;
>>> @@
>>> -    pci_set_dma_mask(e1, e2)
>>> +    dma_set_mask(&e1->dev, e2)
>>>
>>> @@
>>> expression e1, e2;
>>> @@
>>> -    pci_set_consistent_dma_mask(e1, e2)
>>> +    dma_set_coherent_mask(&e1->dev, e2)
>>>
>>> While at it, some 'dma_set_mask()/dma_set_coherent_mask()' have been
>>> updated to a much less verbose 'dma_set_mask_and_coherent()'.
>>>
>>> Signed-off-by: Wang Qing <wangqing@...o.com>
>>> ---
>>>    drivers/dma/dw-edma/dw-edma-pcie.c | 17 ++++-------------
>>>    1 file changed, 4 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
>>> index 44f6e09..198f6cd
>>> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
>>> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
>>> @@ -186,27 +186,18 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>>>    	pci_set_master(pdev);
>>>    	/* DMA configuration */
>>> -	err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64));
>>> +	err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
>>>    	if (!err) {
>> if err = 0, so if no error...
>>
>>> -		err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64));
>>> -		if (err) {
>>> -			pci_err(pdev, "consistent DMA mask 64 set failed\n");
>>> -			return err;
>>> -		}
>>> +		pci_err(pdev, "DMA mask 64 set failed\n");
>>> +		return err;
>> ... we log an error, return success but don't perform the last steps of the
>> probe.
> 
> I have an unpublished Smatch check for these:
> 
> drivers/dma/dw-edma/dw-edma-pcie.c:192 dw_edma_pcie_probe() info: return a literal instead of 'err'
> 
> The idea of the Smatch check is that it's pretty easy to get "if (!ret)"
> and "if (ret)" transposed.  It would show up in testing, of course, but
> the truth is that maintainers don't always have all the hardware they
> maintain.
> 
> And the other idea is that "return 0;" is always more readable and
> intentional than "return ret;" where ret is zero.
> 
> Anyway, is someone going to fix these?

Patch sent.
Feed-back welcomed.

CJ

> 
> regards,
> dan carpenter
> 
>