Re: [PATCH] dmaengine: switch from 'pci_' to 'dma_' API

[Christophe JAILLET]

Le 23/08/2021 à 09:29, Andy Shevchenko a écrit :
> On Sun, Aug 22, 2021 at 02:40:22PM +0200, Christophe JAILLET wrote:
> > The wrappers in include/linux/pci-dma-compat.h should go away.
> >
> > The patch has been generated with the coccinelle script below.
> >
> > It has been hand modified to use 'dma_set_mask_and_coherent()' instead of
> > 'pci_set_dma_mask()/pci_set_consistent_dma_mask()' when applicable.
> > This is less verbose.
> >
> > It has been compile tested.
>
> > @@
> > expression e1, e2;
> > @@
> > -    pci_set_consistent_dma_mask(e1, e2)
> > +    dma_set_coherent_mask(&e1->dev, e2)
>
> Can we, please, replace this long noise in the commit message with a link to a
> script in coccinelle data base?

Hi,

There is no script in the coccinelle data base up to now, and there is 
no point in adding one now.
The goal of these patches is to remove a deprecated API, so when the job 
will be finished, this script would be of no use and would be removed.

However, I agree that the script as-is is noisy.

I'll replace it with a link to a message already available in lore.

> And the same comment for any future submission that are based on the scripts
> (esp. coccinelle ones).

I usually don't add my coccinelle scripts in the log, but I've been told 
times ago that adding them was a good practice (that I have never 
followed...).

In this particular case, I thought it was helpful for a reviewer to see 
how the automated part had been processed.

> ...
>
> > This patch is mostly mechanical and compile tested. I hope it is ok to
> > update the "drivers/dma/" directory all at once.
>
> There is another discussion with Hellwig [1] about 64-bit DMA mask,
> i.e. it doesn't fail anymore,

Yes, I'm aware of this thread.

I've not taken it into account for 2 reasons:
   - it goes beyond the goal of these patches (i.e. the removal of a 
deprecated API)
   - I *was* not 100% confident about [1].

I *was* giving credit to comment such as [2]. And the pattern "if 64 
bits fails, then switch to 32 bits" is really common.
Maybe it made sense in the past and has remained as-is.


However, since then I've looked at all the architecture specific 
implementation of 'dma_supported()' and [1] looks indeed correct :)


I propose to make these changes in another serie which will mention [1] 
and see the acceptance rate in the different subsystems. (i.e. even if 
the patch is correct, removing what looks like straightforward code may 
puzzle a few of us)

I would start it once "pci-dma-compat.h" has been removed.

Do you agree, or do you want it integrated in the WIP?

Anyway, thanks for the review and comments.

CJ

> so you need to rework drivers accordingly.
>
> [1]: https://lkml.org/lkml/2021/6/7/398

[2]: 
https://elixir.bootlin.com/linux/v5.14-rc7/source/drivers/infiniband/hw/hfi1/pcie.c#L98