[1/2] misc: hpilo: switch from 'pci_' to 'dma_' API
diff mbox series

Message ID 20200718070224.337964-1-christophe.jaillet@wanadoo.fr
State Accepted
Commit 146f90afed258b409104aa6ecc421b823ea1b406
Headers show
Series
  • [1/2] misc: hpilo: switch from 'pci_' to 'dma_' API
Related show

Commit Message

Christophe JAILLET July 18, 2020, 7:02 a.m. UTC
The wrappers in include/linux/pci-dma-compat.h should go away.

The patch has been generated with the coccinelle script below and has been
hand modified to replace GFP_ with a correct flag.
It has been compile tested.

When memory is allocated in 'ilo_ccb_setup()' GFP_ATOMIC must be used
because a spin_lock is hold in 'ilo_open()' before calling
'ilo_ccb_setup()'

@@
@@
-    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
---
 drivers/misc/hpilo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Greg KH July 23, 2020, 7:34 a.m. UTC | #1
On Sat, Jul 18, 2020 at 09:02:24AM +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 and has been
> hand modified to replace GFP_ with a correct flag.
> It has been compile tested.
> 
> When memory is allocated in 'ilo_ccb_setup()' GFP_ATOMIC must be used
> because a spin_lock is hold in 'ilo_open()' before calling
> 'ilo_ccb_setup()'
> 
> @@
> @@
> -    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
> ---
>  drivers/misc/hpilo.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> index 10c975662f8b..c9539c89a925 100644
> --- a/drivers/misc/hpilo.c
> +++ b/drivers/misc/hpilo.c
> @@ -256,7 +256,8 @@ static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
>  	memset_io(device_ccb, 0, sizeof(struct ccb));
>  
>  	/* free resources used to back send/recv queues */
> -	pci_free_consistent(pdev, data->dma_size, data->dma_va, data->dma_pa);
> +	dma_free_coherent(&pdev->dev, data->dma_size, data->dma_va,
> +			  data->dma_pa);
>  }
>  
>  static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
> @@ -272,8 +273,8 @@ static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
>  			 2 * desc_mem_sz(NR_QENTRY) +
>  			 ILO_START_ALIGN + ILO_CACHE_SZ;
>  
> -	data->dma_va = pci_alloc_consistent(hw->ilo_dev, data->dma_size,
> -					    &data->dma_pa);
> +	data->dma_va = dma_alloc_coherent(&hw->ilo_dev->dev, data->dma_size,
> +					  &data->dma_pa, GFP_ATOMIC);

This is being called from open() so it can be GFP_KERNEL.  Can you fix
that up and resend a new version?

thanks,

greg k-h
Christophe JAILLET July 23, 2020, 8:59 a.m. UTC | #2
Le 23/07/2020 à 09:34, Greg KH a écrit :
> On Sat, Jul 18, 2020 at 09:02:24AM +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 and has been
>> hand modified to replace GFP_ with a correct flag.
>> It has been compile tested.
>>
>> When memory is allocated in 'ilo_ccb_setup()' GFP_ATOMIC must be used
>> because a spin_lock is hold in 'ilo_open()' before calling
>> 'ilo_ccb_setup()'

        ^
        |

>> [...]
>>
>> 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
>> ---
>>   drivers/misc/hpilo.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
>> index 10c975662f8b..c9539c89a925 100644
>> --- a/drivers/misc/hpilo.c
>> +++ b/drivers/misc/hpilo.c
>> @@ -256,7 +256,8 @@ static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
>>   	memset_io(device_ccb, 0, sizeof(struct ccb));
>>   
>>   	/* free resources used to back send/recv queues */
>> -	pci_free_consistent(pdev, data->dma_size, data->dma_va, data->dma_pa);
>> +	dma_free_coherent(&pdev->dev, data->dma_size, data->dma_va,
>> +			  data->dma_pa);
>>   }
>>   
>>   static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
>> @@ -272,8 +273,8 @@ static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
>>   			 2 * desc_mem_sz(NR_QENTRY) +
>>   			 ILO_START_ALIGN + ILO_CACHE_SZ;
>>   
>> -	data->dma_va = pci_alloc_consistent(hw->ilo_dev, data->dma_size,
>> -					    &data->dma_pa);
>> +	data->dma_va = dma_alloc_coherent(&hw->ilo_dev->dev, data->dma_size,
>> +					  &data->dma_pa, GFP_ATOMIC);
> 
> This is being called from open() so it can be GFP_KERNEL.  Can you fix
> that up and resend a new version?
> 
> thanks,
> 
> greg k-h
> 

The call chain is:
    .open	                       (file_operations)
       --> ilo_open
	  spin_lock(&hw->open_lock);   (around line 782)
          --> ilo_ccb_setup	       (hw->open_lock is still hold)

So I think that GFP_ATOMIC is needed here, or the code should be 
reworked to avoid holding the spin_lock when ilo_ccb_setup is called.

CJ
Greg KH July 23, 2020, 10:56 a.m. UTC | #3
On Thu, Jul 23, 2020 at 10:59:49AM +0200, Christophe JAILLET wrote:
> Le 23/07/2020 à 09:34, Greg KH a écrit :
> > On Sat, Jul 18, 2020 at 09:02:24AM +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 and has been
> > > hand modified to replace GFP_ with a correct flag.
> > > It has been compile tested.
> > > 
> > > When memory is allocated in 'ilo_ccb_setup()' GFP_ATOMIC must be used
> > > because a spin_lock is hold in 'ilo_open()' before calling
> > > 'ilo_ccb_setup()'
> 
>        ^
>        |

{sigh} see how well I read changelog text...

> 
> > > [...]
> > > 
> > > 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
> > > ---
> > >   drivers/misc/hpilo.c | 7 ++++---
> > >   1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> > > index 10c975662f8b..c9539c89a925 100644
> > > --- a/drivers/misc/hpilo.c
> > > +++ b/drivers/misc/hpilo.c
> > > @@ -256,7 +256,8 @@ static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
> > >   	memset_io(device_ccb, 0, sizeof(struct ccb));
> > >   	/* free resources used to back send/recv queues */
> > > -	pci_free_consistent(pdev, data->dma_size, data->dma_va, data->dma_pa);
> > > +	dma_free_coherent(&pdev->dev, data->dma_size, data->dma_va,
> > > +			  data->dma_pa);
> > >   }
> > >   static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
> > > @@ -272,8 +273,8 @@ static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
> > >   			 2 * desc_mem_sz(NR_QENTRY) +
> > >   			 ILO_START_ALIGN + ILO_CACHE_SZ;
> > > -	data->dma_va = pci_alloc_consistent(hw->ilo_dev, data->dma_size,
> > > -					    &data->dma_pa);
> > > +	data->dma_va = dma_alloc_coherent(&hw->ilo_dev->dev, data->dma_size,
> > > +					  &data->dma_pa, GFP_ATOMIC);
> > 
> > This is being called from open() so it can be GFP_KERNEL.  Can you fix
> > that up and resend a new version?
> > 
> > thanks,
> > 
> > greg k-h
> > 
> 
> The call chain is:
>    .open	                       (file_operations)
>       --> ilo_open
> 	  spin_lock(&hw->open_lock);   (around line 782)
>          --> ilo_ccb_setup	       (hw->open_lock is still hold)
> 
> So I think that GFP_ATOMIC is needed here, or the code should be reworked to
> avoid holding the spin_lock when ilo_ccb_setup is called.

Ok, fair enough, this patch is ok as-is, I'll go queue it up.  Further
fixes for this to move the spinlock out from this is probably a good
idea, if anyone cares about this driver anymore.

thanks,

greg k-h

Patch
diff mbox series

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 10c975662f8b..c9539c89a925 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -256,7 +256,8 @@  static void ilo_ccb_close(struct pci_dev *pdev, struct ccb_data *data)
 	memset_io(device_ccb, 0, sizeof(struct ccb));
 
 	/* free resources used to back send/recv queues */
-	pci_free_consistent(pdev, data->dma_size, data->dma_va, data->dma_pa);
+	dma_free_coherent(&pdev->dev, data->dma_size, data->dma_va,
+			  data->dma_pa);
 }
 
 static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
@@ -272,8 +273,8 @@  static int ilo_ccb_setup(struct ilo_hwinfo *hw, struct ccb_data *data, int slot)
 			 2 * desc_mem_sz(NR_QENTRY) +
 			 ILO_START_ALIGN + ILO_CACHE_SZ;
 
-	data->dma_va = pci_alloc_consistent(hw->ilo_dev, data->dma_size,
-					    &data->dma_pa);
+	data->dma_va = dma_alloc_coherent(&hw->ilo_dev->dev, data->dma_size,
+					  &data->dma_pa, GFP_ATOMIC);
 	if (!data->dma_va)
 		return -ENOMEM;