Message-ID: <alpine.DEB.2.22.394.2108232145590.17496@hadrien>
Date:   Mon, 23 Aug 2021 21:47:44 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        gustavo.pimentel@...opsys.com, vkoul@...nel.org,
        vireshk@...nel.org, wangzhou1@...ilicon.com, logang@...tatee.com,
        dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] dmaengine: switch from 'pci_' to 'dma_' API



On Mon, 23 Aug 2021, Christophe JAILLET wrote:

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

You can perhaps include a script that represents a very typical case or
the specific case that is relevant to the patch.

julia


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

Powered by blists - more mailing lists