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

[Christophe JAILLET]

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@xxxxxxxx>
> > >
> > > 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@xxxxxxxx>
> > > ---
> > >    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