| Message ID | 547fae4abef1ca3bf2198ca68e6c361b4d02f13c.1629635852.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | dmaengine: switch from 'pci_' to 'dma_' API | expand |
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? And the same comment for any future submission that are based on the scripts (esp. coccinelle ones). ... > 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, so you need to rework drivers accordingly. [1]: https://lkml.org/lkml/2021/6/7/398
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
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 >
Le 23/08/2021 à 21:47, Julia Lawall a écrit : > > > 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 > > Hi julia, I still have to post ~9 of such patches. I'll try your proposal to only include relevant part of the script. It should be do-able. The most puzzling in the remaining files is: drivers/char/xillybus/xillybus_pcie.c In this driver, 'xilly_pci_direction()' looks odd and somewhat useless (but I'll let it more or less as-is. It may catch corner cases) What puzzles me is "struct xilly_mapping" where the "void *" seems to hold different kinds of pointer depending of the context. I could add some casting but would prefer to avoid this and have a cleaner solution. I've not look in detail yet but my first attempt were not very conclusive. "message/fusion" has not been Acked yet. All patches have been posted but there are huge and I can easily understand that reviewing them is complex because of the GFP_KERNEL I introduce. Only one has been merged yet. Moreover, according to -next logs, no single other patch has been merged since end of June. CJ
diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c index 44f6e09bdb53..1421bc9f3724 100644 --- a/drivers/dma/dw-edma/dw-edma-pcie.c +++ b/drivers/dma/dw-edma/dw-edma-pcie.c @@ -186,27 +186,15 @@ 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)); - if (!err) { - 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; - } - } else { + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + if (err) { pci_err(pdev, "DMA mask 64 set failed\n"); - err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (err) { pci_err(pdev, "DMA mask 32 set failed\n"); return err; } - - err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); - if (err) { - pci_err(pdev, "consistent DMA mask 32 set failed\n"); - return err; - } } /* Data structure allocation */ diff --git a/drivers/dma/dw/pci.c b/drivers/dma/dw/pci.c index 26a3f926da02..ad2d4d012cf7 100644 --- a/drivers/dma/dw/pci.c +++ b/drivers/dma/dw/pci.c @@ -32,11 +32,7 @@ static int dw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *pid) pci_set_master(pdev); pci_try_set_mwi(pdev); - ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); - if (ret) - return ret; - - ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (ret) return ret; diff --git a/drivers/dma/hisi_dma.c b/drivers/dma/hisi_dma.c index c855a0e4f9ff..97c87a7cba87 100644 --- a/drivers/dma/hisi_dma.c +++ b/drivers/dma/hisi_dma.c @@ -519,11 +519,7 @@ static int hisi_dma_probe(struct pci_dev *pdev, const struct pci_device_id *id) return ret; } - ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(64)); - if (ret) - return ret; - - ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)); + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); if (ret) return ret; diff --git a/drivers/dma/hsu/pci.c b/drivers/dma/hsu/pci.c index 9045a6f7f589..6a2df3dd78d0 100644 --- a/drivers/dma/hsu/pci.c +++ b/drivers/dma/hsu/pci.c @@ -65,11 +65,7 @@ static int hsu_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) pci_set_master(pdev); pci_try_set_mwi(pdev); - ret = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); - if (ret) - return ret; - - ret = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (ret) return ret; diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c index 191b59279007..373b8dac6c9b 100644 --- a/drivers/dma/ioat/init.c +++ b/drivers/dma/ioat/init.c @@ -1363,15 +1363,9 @@ static int ioat_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (!iomap) return -ENOMEM; - err = pci_set_dma_mask(pdev, DMA_BIT_MASK(64)); + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); if (err) - err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); - if (err) - return err; - - err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(64)); - if (err) - err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); + err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (err) return err; diff --git a/drivers/dma/pch_dma.c b/drivers/dma/pch_dma.c index 1da04112fcdb..c359decc07a3 100644 --- a/drivers/dma/pch_dma.c +++ b/drivers/dma/pch_dma.c @@ -835,7 +835,7 @@ static int pch_dma_probe(struct pci_dev *pdev, goto err_disable_pdev; } - err = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); if (err) { dev_err(&pdev->dev, "Cannot set proper DMA config\n"); goto err_free_res; diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c index 166934544161..1ffcb5ca9788 100644 --- a/drivers/dma/plx_dma.c +++ b/drivers/dma/plx_dma.c @@ -563,15 +563,9 @@ static int plx_dma_probe(struct pci_dev *pdev, if (rc) return rc; - rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(48)); + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(48)); if (rc) - rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(32)); - if (rc) - return rc; - - rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(48)); - if (rc) - rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(32)); + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); if (rc) return rc;
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. @@ @@ - PCI_DMA_BIDIRECTIONAL + DMA_BIDIRECTIONAL @@ @@ - PCI_DMA_TODEVICE + DMA_TO_DEVICE @@ @@ - PCI_DMA_FROMDEVICE + DMA_FROM_DEVICE @@ @@ - PCI_DMA_NONE + DMA_NONE @@ expression e1, e2, e3; @@ - pci_alloc_consistent(e1, e2, e3) + dma_alloc_coherent(&e1->dev, e2, e3, GFP_) @@ expression e1, e2, e3; @@ - pci_zalloc_consistent(e1, e2, e3) + dma_alloc_coherent(&e1->dev, e2, e3, GFP_) @@ expression e1, e2, e3, e4; @@ - pci_free_consistent(e1, e2, e3, e4) + dma_free_coherent(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_map_single(e1, e2, e3, e4) + dma_map_single(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_unmap_single(e1, e2, e3, e4) + dma_unmap_single(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4, e5; @@ - pci_map_page(e1, e2, e3, e4, e5) + dma_map_page(&e1->dev, e2, e3, e4, e5) @@ expression e1, e2, e3, e4; @@ - pci_unmap_page(e1, e2, e3, e4) + dma_unmap_page(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_map_sg(e1, e2, e3, e4) + dma_map_sg(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_unmap_sg(e1, e2, e3, e4) + dma_unmap_sg(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_dma_sync_single_for_cpu(e1, e2, e3, e4) + dma_sync_single_for_cpu(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_dma_sync_single_for_device(e1, e2, e3, e4) + dma_sync_single_for_device(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_dma_sync_sg_for_cpu(e1, e2, e3, e4) + dma_sync_sg_for_cpu(&e1->dev, e2, e3, e4) @@ expression e1, e2, e3, e4; @@ - pci_dma_sync_sg_for_device(e1, e2, e3, e4) + dma_sync_sg_for_device(&e1->dev, e2, e3, e4) @@ expression e1, e2; @@ - pci_dma_mapping_error(e1, e2) + dma_mapping_error(&e1->dev, e2) @@ 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) Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- If needed, see post from Christoph Hellwig on the kernel-janitors ML: https://marc.info/?l=kernel-janitors&m=158745678307186&w=4 This patch is mostly mechanical and compile tested. I hope it is ok to update the "drivers/dma/" directory all at once. --- drivers/dma/dw-edma/dw-edma-pcie.c | 18 +++--------------- drivers/dma/dw/pci.c | 6 +----- drivers/dma/hisi_dma.c | 6 +----- drivers/dma/hsu/pci.c | 6 +----- drivers/dma/ioat/init.c | 10 ++-------- drivers/dma/pch_dma.c | 2 +- drivers/dma/plx_dma.c | 10 ++-------- 7 files changed, 11 insertions(+), 47 deletions(-)