new TODO list item

new TODO list item

[Christoph Hellwig]

Hi Janitors,

if someone feels like helping with a fairly trivial legacy API, the
wrappers in include/linux/pci-dma-compat.h should go away.  This is
mostly trivially scriptable, except for dma_alloc_coherent, where
the GFP_ATOMIC passed by pci_alloc_consisteny should usually be replaced
with GFP_KERNEL when not calling from an atomic context.

Re: new TODO list item

[Suraj Upadhyay]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On Tue, Apr 21, 2020 at 10:12:57AM +0200, Christoph Hellwig wrote:
> Hi Janitors,
> 
> if someone feels like helping with a fairly trivial legacy API, the
> wrappers in include/linux/pci-dma-compat.h should go away.  This is
> mostly trivially scriptable, except for dma_alloc_coherent, where
> the GFP_ATOMIC passed by pci_alloc_consisteny should usually be replaced
> with GFP_KERNEL when not calling from an atomic context.

Hii Christoph,
	This is my first time posting to kernel-janitors. I would be glad to
help with this task but suggest me if I should get started with
something else.

Regards,

Suraj Upadhyay

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: new TODO list item

[Christoph Hellwig]

On Tue, Apr 21, 2020 at 01:53:25PM +0530, Suraj Upadhyay wrote:
> On Tue, Apr 21, 2020 at 10:12:57AM +0200, Christoph Hellwig wrote:
> > Hi Janitors,
> > 
> > if someone feels like helping with a fairly trivial legacy API, the
> > wrappers in include/linux/pci-dma-compat.h should go away.  This is
> > mostly trivially scriptable, except for dma_alloc_coherent, where
> > the GFP_ATOMIC passed by pci_alloc_consisteny should usually be replaced
> > with GFP_KERNEL when not calling from an atomic context.
> 
> Hii Christoph,
> 	This is my first time posting to kernel-janitors. I would be glad to
> help with this task but suggest me if I should get started with
> something else.

Sure, feel free to get started.  A coccinelle script to do the grunt
work might be useful, though.

Re: new TODO list item

[Christophe JAILLET]

Le 21/04/2020 à 10:12, Christoph Hellwig a écrit :
> Hi Janitors,
> 
> if someone feels like helping with a fairly trivial legacy API, the
> wrappers in include/linux/pci-dma-compat.h should go away.  This is
> mostly trivially scriptable, except for dma_alloc_coherent, where
> the GFP_ATOMIC passed by pci_alloc_consisteny should usually be replaced
> with GFP_KERNEL when not calling from an atomic context.
> 

Hi,
what would be the best approach to work on, it?

I've processed the current tree, with the coccinelle script below.
For 'dma_alloc_coherent' calls, I've left a GFP_ on purpose, so that one 
need to wonder which flag is the best.

'make'ing the files before sending patches should spot the places were a 
correct flag has not been defined.

What puzzles me is that the script below >20k lines file and the 
diffstat is:
     288 files changed, 3963 insertions(+), 3857 deletions(-)


1. Does sending patches one file at a time makes sense?
2. Should the PCI_DMA_ --> DMA_ conversion should be handled first? (the 
#defined values are the same, it should be straightforward)
3. Should a huge patch series be sent to fix all at once?
4. Should we update everything except 'dma_alloc_coherent' all at once, 
then one file at a time for the allocation with correct GFP_ flag?


I could help with 1. or 2nd step of 4. This could go in the right 
direction, but would require time.

Other tree wide approaches look complex to me.


CJ


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

----------------------------------------

Re: new TODO list item

[Christoph Hellwig]

On Mon, Jun 22, 2020 at 08:32:36PM +0200, Christophe JAILLET wrote:
> Le 21/04/2020 ?? 10:12, Christoph Hellwig a ??crit??:
> > Hi Janitors,
> > 
> > if someone feels like helping with a fairly trivial legacy API, the
> > wrappers in include/linux/pci-dma-compat.h should go away.  This is
> > mostly trivially scriptable, except for dma_alloc_coherent, where
> > the GFP_ATOMIC passed by pci_alloc_consisteny should usually be replaced
> > with GFP_KERNEL when not calling from an atomic context.
> > 
> 
> Hi,
> what would be the best approach to work on, it?
> 
> I've processed the current tree, with the coccinelle script below.
> For 'dma_alloc_coherent' calls, I've left a GFP_ on purpose, so that one
> need to wonder which flag is the best.
> 
> 'make'ing the files before sending patches should spot the places were a
> correct flag has not been defined.
> 
> What puzzles me is that the script below >20k lines file and the diffstat
> is:
>     288 files changed, 3963 insertions(+), 3857 deletions(-)
> 
> 
> 1. Does sending patches one file at a time makes sense?

File is a little too small.  Split on a subsystem, or for subsystems
that have a lot of drivers (e.g. scsi or net) on  per-driver basis.

> 2. Should the PCI_DMA_ --> DMA_ conversion should be handled first? (the
> #defined values are the same, it should be straightforward)

I wouldn't bother.

> 3. Should a huge patch series be sent to fix all at once?

I don't think that is helpful.  Just one series (or single patch)
per subsystem.

> 4. Should we update everything except 'dma_alloc_coherent' all at once, then
> one file at a time for the allocation with correct GFP_ flag?

I don't think so.

Btw, we have an issue with drivers/message/fusion where the GFP_ATOMIC
removal would be really useful as in fixing a regression caused by
another change. Can you do that as a trial ASAP and also Cc
Robin Murphy <robin.murphy@arm.com>, Guenter Roeck <linux@roeck-us.net>
and Geert Uytterhoeven <geert@linux-m68k.org>

Re: new TODO list item

[Suraj Upadhyay]

[-- Attachment #1.1: Type: text/plain, Size: 1717 bytes --]

On Tue, Apr 21, 2020 at 04:26:16AM -0700, Christoph Hellwig wrote:
> On Tue, Apr 21, 2020 at 01:53:25PM +0530, Suraj Upadhyay wrote:
> > On Tue, Apr 21, 2020 at 10:12:57AM +0200, Christoph Hellwig wrote:
> > > Hi Janitors,
> > > 
> > > if someone feels like helping with a fairly trivial legacy API, the
> > > wrappers in include/linux/pci-dma-compat.h should go away.  This is
> > > mostly trivially scriptable, except for dma_alloc_coherent, where
> > > the GFP_ATOMIC passed by pci_alloc_consisteny should usually be replaced
> > > with GFP_KERNEL when not calling from an atomic context.
> > 
> > Hii Christoph,
> > 	This is my first time posting to kernel-janitors. I would be glad to
> > help with this task but suggest me if I should get started with
> > something else.
> 
> Sure, feel free to get started.  A coccinelle script to do the grunt
> work might be useful, though.

Hii Christoph,
	I nearly forgot about this, and now I would like to complete
this task.
However, even after two months I am still a newbie and there are a few
questions that I would like to ask :

1.	Multiple files will be affected by this change, who do you think 
	I should send my patches to ??
2.	Should I send my patches as a patchset of multiple small patches
	chaging a particular API for e.g. sending all patches that change
	pci_map_single() to dma_map_single() in one file in the same patchset ??
3.	I literally have little idea on how to change the comments and
	docs which refer to these APIs.
4.	And what if a maintainer likes the previous APIs for simplicity
	and rejects the patches that try to change them ??

Thanks,

Suraj Upadhyay.

CCing Kernelnewbies@kernelnewbies.org


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: new TODO list item

[Christophe JAILLET]

Le 21/04/2020 à 10:12, Christoph Hellwig a écrit :
> Hi Janitors,
> 
> if someone feels like helping with a fairly trivial legacy API, the
> wrappers in include/linux/pci-dma-compat.h should go away.  This is
> mostly trivially scriptable, except for dma_alloc_coherent, where
> the GFP_ATOMIC passed by pci_alloc_consistent should usually be replaced
> with GFP_KERNEL when not calling from an atomic context.
> 

Hi,
just a small update on the work on progress:

https://elixir.bootlin.com/linux/v5.8/A/ident/pci_alloc_consistent
    --> 88 files
https://elixir.bootlin.com/linux/v5.9/A/ident/pci_alloc_consistent
    --> 62 files
https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_alloc_consistent
    --> 32 files

https://elixir.bootlin.com/linux/v5.8/A/ident/pci_zalloc_consistent
    --> 26 files
https://elixir.bootlin.com/linux/v5.9/A/ident/pci_zalloc_consistent
    --> 19 files
https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_zalloc_consistent
    --> 11 files


None of the drivers/media/pci/* patches has been accepted yet.


CJ

Re: new TODO list item

[Marion & Christophe JAILLET]

  Le 21/04/2020 à 10:12, Christoph Hellwig a écrit :
> Hi Janitors,
>
> if someone feels like helping with a fairly trivial legacy API, the
> wrappers in include/linux/pci-dma-compat.h should go away.  This is
> mostly trivially scriptable, except for dma_alloc_coherent, where
> the GFP_ATOMIC passed by pci_alloc_consistent should usually be replaced
> with GFP_KERNEL when not calling from an atomic context.
>

  Hi,
  just a small update on the work on progress:

  https://elixir.bootlin.com/linux/v5.8/A/ident/pci_alloc_consistent
      --> 88 files
  https://elixir.bootlin.com/linux/v5.9/A/ident/pci_alloc_consistent
      --> 62 files
  https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_alloc_consistent
      --> 32 files
  https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_alloc_consistent
      --> 20 files

  https://elixir.bootlin.com/linux/v5.8/A/ident/pci_zalloc_consistent
      --> 26 files
  https://elixir.bootlin.com/linux/v5.9/A/ident/pci_zalloc_consistent
      --> 19 files
  https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_zalloc_consistent
      --> 11 files
  https://elixir.bootlin.com/linux/v5.11-rc1/A/ident/pci_zalloc_consistent
      --> 4 files


  CJ

Re: new TODO list item

[Marion & Christophe JAILLET]

  Le 21/04/2020 à 10:12, Christoph Hellwig a écrit :
 > Hi Janitors,
 >
 > if someone feels like helping with a fairly trivial legacy API, the
 > wrappers in include/linux/pci-dma-compat.h should go away.  This is
 > mostly trivially scriptable, except for dma_alloc_coherent, where
 > the GFP_ATOMIC passed by pci_alloc_consistent should usually be replaced
 > with GFP_KERNEL when not calling from an atomic context.
 >

  Hi,
  just a small update on the work on progress:

  https://elixir.bootlin.com/linux/v5.8/A/ident/pci_alloc_consistent
      --> 88 files
  https://elixir.bootlin.com/linux/v5.9/A/ident/pci_alloc_consistent
      --> 62 files
  https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_alloc_consistent
      --> 32 files
  https://elixir.bootlin.com/linux/v5.11-rc1/A/ident/pci_alloc_consistent
      --> 20 files
  https://elixir.bootlin.com/linux/v5.12-rc1/A/ident/pci_alloc_consistent
      --> 12 files
  https://elixir.bootlin.com/linux/v5.13-rc1/A/ident/pci_alloc_consistent
      --> 6 files (4 in drivers/message/fusion)

  https://elixir.bootlin.com/linux/v5.8/A/ident/pci_zalloc_consistent
      --> 26 files
  https://elixir.bootlin.com/linux/v5.9/A/ident/pci_zalloc_consistent
      --> 19 files
  https://elixir.bootlin.com/linux/v5.10-rc1/A/ident/pci_zalloc_consistent
      --> 11 files
  https://elixir.bootlin.com/linux/v5.11-rc1/A/ident/pci_zalloc_consistent
      --> 4 files
  https://elixir.bootlin.com/linux/v5.12-rc1/A/ident/pci_zalloc_consistent
      --> 3 files
  https://elixir.bootlin.com/linux/v5.13-rc1/A/ident/pci_zalloc_consistent
      --> 1 file

So, I hope that this job will be finished for the next -rc.


However, any help for 'drivers/message/fusion' would be appreciated.

My strategy to check all callers to be sure that GFP_KERNEL or 
GFP_ATOMIC is fine doesn't work well here.
Someone with a more global view of this (huge) driver would help a lot.

Also, at the beginning, I planned only to make the 
pci_[z]alloc_consistent part. So now that is it mostly done, if anyone 
feel like looking at the other conversions, it should be only some 
mechanical stuff.

CJ