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

[Dan Carpenter <dan.carpenter@oracle.com>]

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@vivo.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@vivo.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