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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 9 Nov 2021 16:21:37 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
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

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?

regards,
dan carpenter