| Message ID | 20200718070224.337964-1-christophe.jaillet@wanadoo.fr |
|---|---|
| State | Accepted |
| Commit | 146f90afed258b409104aa6ecc421b823ea1b406 |
| Headers | show |
| Series |
|
| Related | show |
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
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
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
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;
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(-)