Re: [PATCH 1/9] cmd64x: implement clear_irq() method

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello.

Bartlomiej Zolnierkiewicz wrote:

Convert the driver's two dma_end() methods into clear_irq() methods -- the
driver will now use the standard dma_end() method implementation, ide_dma_end().

Signed-off-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>

---
The patch is atop of ide-2.6.git 'for-next' branch.

 drivers/ide/cmd64x.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)
[...]
@@ -226,11 +226,10 @@ static void cmd64x_set_dma_mode(ide_driv
 		(void) pci_write_config_byte(dev, pciU, regU);
 }
-static int cmd648_dma_end(ide_drive_t *drive)
+static void cmd648_clear_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= drive->hwif;
 	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
Don't we need to check whether hwif->dma_base is valid now?
You're right, I have managed to overlook this. I'll change this to pci_resource_start() call instead...

Currently this driver should operate fine without BAR4 set so even if
this is pci_resource_start(), the return value still needs to be checked
against 0 -- it is the reliability/maintainability issue.

Nay, the PCI device just must not be enabled in this case: all BARs must be allocated, period -- that's what the PCI specs say, IIRC. Besides, 0 doesn't generally mean "unassigned" in the PCI world. I know, I know, we consider it unallocated but that's not correct -- only PCI 2.2 (IIRC) used to have words about 0 meaning "unassigned". Note that we don't check pci_resource_start() result for 0 in the drivers that do call it, like hpt366.c... I'm seeing the checks in pdc202xx_*.c but they seem pretty useless -- this just must not happen. So, I don't agree here...

Also shouldn't this patch be done after fixing core code first?
   Fixing what exactly, ide_timer_expiry()?

Yes.  Currently IRQ is cleared in ide_timer_expiry()'s code-path:

 -> hwif->handler [ ide_dma_intr() ]
     -> dma_ops->dma_end [ cmd64{x,8}_dma_end() ]

After this patch it won't be.

  Ah, that's a correct spotting...

MBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux