The first patch removes deprecated use of the pci api.
The second patch depends on the first, it replaces uses of GFP_ATOMIC
with GFP_KERNEL. I could not find evidence that the concerned functions
were called from a context were sleep is not allowed.
---
cs5520.c | 2 +-
pmac.c | 7 +++----
setup-pci.c | 2 +-
sgiioc4.c | 6 +++---
4 files changed, 8 insertions(+), 9 deletions(-)
Replace occurences of the pci api by appropriate call to the dma api.
A simplified version of the semantic patch that finds this problem is as
follows: (http://coccinelle.lip6.fr)
@deprecated@
idexpression id;
position p;
@@
(
pci_dma_supported@p ( id, ...)
|
pci_alloc_consistent@p ( id, ...)
)
@bad1@
idexpression id;
position deprecated.p;
@@
...when != &id->dev
when != pci_get_drvdata ( id )
when != pci_enable_device ( id )
(
pci_dma_supported@p ( id, ...)
|
pci_alloc_consistent@p ( id, ...)
)
@depends on !bad1@
idexpression id;
expression direction;
position deprecated.p;
@@
(
- pci_dma_supported@p ( id,
+ dma_supported ( &id->dev,
...
+ , GFP_ATOMIC
)
|
- pci_alloc_consistent@p ( id,
+ dma_alloc_coherent ( &id->dev,
...
+ , GFP_ATOMIC
)
)
Signed-off-by: Quentin Lambert <[email protected]>
---
drivers/ide/cs5520.c | 2 +-
drivers/ide/pmac.c | 5 ++---
drivers/ide/setup-pci.c | 2 +-
drivers/ide/sgiioc4.c | 4 ++--
4 files changed, 6 insertions(+), 7 deletions(-)
--- a/drivers/ide/cs5520.c
+++ b/drivers/ide/cs5520.c
@@ -123,7 +123,7 @@ static int cs5520_init_one(struct pci_de
return -ENODEV;
}
pci_set_master(dev);
- if (pci_set_dma_mask(dev, DMA_BIT_MASK(32))) {
+ if (dma_set_mask(&dev->dev, DMA_BIT_MASK(32))) {
printk(KERN_WARNING "%s: No suitable DMA available.\n",
d->name);
return -ENODEV;
--- a/drivers/ide/pmac.c
+++ b/drivers/ide/pmac.c
@@ -1689,10 +1689,9 @@ static int pmac_ide_init_dma(ide_hwif_t
* The +2 is +1 for the stop command and +1 to allow for
* aligning the start address to a multiple of 16 bytes.
*/
- pmif->dma_table_cpu = pci_alloc_consistent(
- dev,
+ pmif->dma_table_cpu = dma_alloc_coherent(&dev->dev,
(MAX_DCMDS + 2) * sizeof(struct dbdma_cmd),
- &hwif->dmatable_dma);
+ &hwif->dmatable_dma, GFP_ATOMIC);
if (pmif->dma_table_cpu == NULL) {
printk(KERN_ERR "%s: unable to allocate DMA command list\n",
hwif->name);
--- a/drivers/ide/setup-pci.c
+++ b/drivers/ide/setup-pci.c
@@ -209,7 +209,7 @@ static int ide_pci_enable(struct pci_dev
* a DMA mask field to the struct ide_port_info if we need it
* (or let lower level driver set the DMA mask)
*/
- ret = pci_set_dma_mask(dev, DMA_BIT_MASK(32));
+ ret = dma_set_mask(&dev->dev, DMA_BIT_MASK(32));
if (ret < 0) {
printk(KERN_ERR "%s %s: can't set DMA mask\n",
d->name, pci_name(dev));
--- a/drivers/ide/sgiioc4.c
+++ b/drivers/ide/sgiioc4.c
@@ -334,8 +334,8 @@ static int ide_dma_sgiioc4(ide_hwif_t *h
if (ide_allocate_dma_engine(hwif))
goto dma_pci_alloc_failure;
- pad = pci_alloc_consistent(dev, IOC4_IDE_CACHELINE_SIZE,
- (dma_addr_t *)&hwif->extra_base);
+ pad = dma_alloc_coherent(&dev->dev, IOC4_IDE_CACHELINE_SIZE,
+ (dma_addr_t *)&hwif->extra_base, GFP_ATOMIC);
if (pad) {
ide_set_hwifdata(hwif, pad);
return 0;
Both pmac_ide_init_dma and ide_dma_sgiioc4 are stored in the init_dma field of
an ide_port_info structure. This field seems to only be called from contexts
where sleep is allowed. Therefore, this patch replaces uses of GFP_ATOMIC by
GFP_KERNEL.
Signed-off-by: Quentin Lambert <[email protected]>
---
drivers/ide/pmac.c | 2 +-
drivers/ide/sgiioc4.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--- a/drivers/ide/pmac.c
+++ b/drivers/ide/pmac.c
@@ -1691,7 +1691,7 @@ static int pmac_ide_init_dma(ide_hwif_t
*/
pmif->dma_table_cpu = dma_alloc_coherent(&dev->dev,
(MAX_DCMDS + 2) * sizeof(struct dbdma_cmd),
- &hwif->dmatable_dma, GFP_ATOMIC);
+ &hwif->dmatable_dma, GFP_KERNEL);
if (pmif->dma_table_cpu == NULL) {
printk(KERN_ERR "%s: unable to allocate DMA command list\n",
hwif->name);
--- a/drivers/ide/sgiioc4.c
+++ b/drivers/ide/sgiioc4.c
@@ -335,7 +335,7 @@ static int ide_dma_sgiioc4(ide_hwif_t *h
goto dma_pci_alloc_failure;
pad = dma_alloc_coherent(&dev->dev, IOC4_IDE_CACHELINE_SIZE,
- (dma_addr_t *)&hwif->extra_base, GFP_ATOMIC);
+ (dma_addr_t *)&hwif->extra_base, GFP_KERNEL);
if (pad) {
ide_set_hwifdata(hwif, pad);
return 0;
Why are these GFP_ATOMIC allocations?
regards,
dan carpenter
On Thu, Apr 09, 2015 at 01:46:28PM +0200, Quentin Lambert wrote:
> Both pmac_ide_init_dma and ide_dma_sgiioc4 are stored in the init_dma field of
> an ide_port_info structure. This field seems to only be called from contexts
> where sleep is allowed. Therefore, this patch replaces uses of GFP_ATOMIC by
> GFP_KERNEL.
>
> Signed-off-by: Quentin Lambert <[email protected]>
Oh. They're not GFP_ATOMIC.
Fold these two patches together into one patch and resend.
regards,
dan carpenter
On 09/04/2015 14:36, Dan Carpenter wrote:
> Oh. They're not GFP_ATOMIC.
>
> Fold these two patches together into one patch and resend.
The reason I did it that way is because I feel that the two patches
really are different.
The first one do not change the execution of the code but the second one
does.
Could you explain to me why they can be folded into one ?
regards,
Quentin
On Thu, Apr 09, 2015 at 04:33:47PM +0200, Quentin Lambert wrote:
>
>
> On 09/04/2015 14:36, Dan Carpenter wrote:
> >Oh. They're not GFP_ATOMIC.
> >
> >Fold these two patches together into one patch and resend.
> The reason I did it that way is because I feel that the two patches
> really are different.
> The first one do not change the execution of the code but the second
> one does.
> Could you explain to me why they can be folded into one ?
When I read the first patch it left me with unanswered questions and I
was planning to ask you to redo it. After you had fixed 1/1 there is no
need for 2/2.
regards,
dan carpenter
Sorry, my last email was bad.
Splitting patches into logical parts is a bit tricky. Let me try
explain better.
Every patch should sort of make sense on its own. In the original code
it's using GFP_ATOMIC but that's because the original API was bad and
we had no choice. In the 1/1 patch we're using GFP_ATOMIC explicitly
by choice and it's wrong. In patch 2/2 we fix this problem but we
shouldn't introduce bad code even if we fix it in later patches.
regards,
dan carpenter
On 09/04/2015 16:50, Dan Carpenter wrote:
> Sorry, my last email was bad.
>
> Splitting patches into logical parts is a bit tricky. Let me try
> explain better.
>
> Every patch should sort of make sense on its own. In the original code
> it's using GFP_ATOMIC but that's because the original API was bad and
> we had no choice. In the 1/1 patch we're using GFP_ATOMIC explicitly
> by choice and it's wrong. In patch 2/2 we fix this problem but we
> shouldn't introduce bad code even if we fix it in later patches.
ok thanks
>
> regards,
> dan carpenter
>
On Thu, 9 Apr 2015, Dan Carpenter wrote:
> Sorry, my last email was bad.
>
> Splitting patches into logical parts is a bit tricky. Let me try
> explain better.
>
> Every patch should sort of make sense on its own. In the original code
> it's using GFP_ATOMIC but that's because the original API was bad and
> we had no choice. In the 1/1 patch we're using GFP_ATOMIC explicitly
> by choice and it's wrong. In patch 2/2 we fix this problem but we
> shouldn't introduce bad code even if we fix it in later patches.
But if Quentin's analysis is wrong, then we have to undo the GFP_KERNEL
choice, and with only one patch we end up back at the pci API?
julia
On Thu, Apr 09, 2015 at 04:53:48PM +0200, Julia Lawall wrote:
>
>
> On Thu, 9 Apr 2015, Dan Carpenter wrote:
>
> > Sorry, my last email was bad.
> >
> > Splitting patches into logical parts is a bit tricky. Let me try
> > explain better.
> >
> > Every patch should sort of make sense on its own. In the original code
> > it's using GFP_ATOMIC but that's because the original API was bad and
> > we had no choice. In the 1/1 patch we're using GFP_ATOMIC explicitly
> > by choice and it's wrong. In patch 2/2 we fix this problem but we
> > shouldn't introduce bad code even if we fix it in later patches.
>
> But if Quentin's analysis is wrong, then we have to undo the GFP_KERNEL
> choice, and with only one patch we end up back at the pci API?
We still only have to revert one patch either way.
regards,
dan carpenter
On Thu, 9 Apr 2015 13:46:27 +0200
Quentin Lambert <[email protected]> wrote:
> Replace occurences of the pci api by appropriate call to the dma api.
>
Drivers/ide is obsolete. It probably ought to go at this point but even
so it exists solely in case of a problem or compatibility issue with
drivers/ata. Cleaning it up without testing all the drivers makes no
sense IMHO. Cleaning it up and testing them doesn't make much sense
either.
Alan
From: One Thousand Gnomes <[email protected]>
Date: Mon, 13 Apr 2015 13:25:30 +0100
> On Thu, 9 Apr 2015 13:46:27 +0200
> Quentin Lambert <[email protected]> wrote:
>
>> Replace occurences of the pci api by appropriate call to the dma api.
>>
>
> Drivers/ide is obsolete. It probably ought to go at this point but even
> so it exists solely in case of a problem or compatibility issue with
> drivers/ata. Cleaning it up without testing all the drivers makes no
> sense IMHO. Cleaning it up and testing them doesn't make much sense
> either.
It's a straightforward transformation, that actually evaluates to what
the definitions of the deprecated pci DMA interfaces are defined to.
So I intend to apply this patch, especially for the sake of allowing
these PCI DMA interfaces to be removed.
On Mon, Apr 13, 2015 at 01:08:13PM -0400, David Miller wrote:
> From: One Thousand Gnomes <[email protected]>
> Date: Mon, 13 Apr 2015 13:25:30 +0100
>
> > On Thu, 9 Apr 2015 13:46:27 +0200
> > Quentin Lambert <[email protected]> wrote:
> >
> >> Replace occurences of the pci api by appropriate call to the dma api.
> >>
> >
> > Drivers/ide is obsolete. It probably ought to go at this point but even
> > so it exists solely in case of a problem or compatibility issue with
> > drivers/ata. Cleaning it up without testing all the drivers makes no
> > sense IMHO. Cleaning it up and testing them doesn't make much sense
> > either.
>
> It's a straightforward transformation, that actually evaluates to what
> the definitions of the deprecated pci DMA interfaces are defined to.
Almost, except that it uses GFP_KERNEL instead of GFP_DMA. But I
reviewed it and I think it's fine. Quentin sent a GFP_ATOMIC patch as
the first version and I asked him to redo it. Maybe I shouldn't have
done that...
regards,
dan carpenter
From: Dan Carpenter <[email protected]>
Date: Mon, 13 Apr 2015 20:23:24 +0300
> On Mon, Apr 13, 2015 at 01:08:13PM -0400, David Miller wrote:
>> From: One Thousand Gnomes <[email protected]>
>> Date: Mon, 13 Apr 2015 13:25:30 +0100
>>
>> > On Thu, 9 Apr 2015 13:46:27 +0200
>> > Quentin Lambert <[email protected]> wrote:
>> >
>> >> Replace occurences of the pci api by appropriate call to the dma api.
>> >>
>> >
>> > Drivers/ide is obsolete. It probably ought to go at this point but even
>> > so it exists solely in case of a problem or compatibility issue with
>> > drivers/ata. Cleaning it up without testing all the drivers makes no
>> > sense IMHO. Cleaning it up and testing them doesn't make much sense
>> > either.
>>
>> It's a straightforward transformation, that actually evaluates to what
>> the definitions of the deprecated pci DMA interfaces are defined to.
>
> Almost, except that it uses GFP_KERNEL instead of GFP_DMA. But I
> reviewed it and I think it's fine. Quentin sent a GFP_ATOMIC patch as
> the first version and I asked him to redo it. Maybe I shouldn't have
> done that...
No, I'm fine with that part too, don't worry.