| Message ID | 1428579988-10167-3-git-send-email-lambert.quentin@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
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 <lambert.quentin@gmail.com> Oh. They're not GFP_ATOMIC. Fold these two patches together into one patch and resend. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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;
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 <lambert.quentin@gmail.com> --- drivers/ide/pmac.c | 2 +- drivers/ide/sgiioc4.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html