On Thursday 10 July 2008, Sergei Shtylyov wrote: > Hello, I wrote: > > >> Index: b/drivers/ide/ide-iops.c > >> =================================================================== > >> --- a/drivers/ide/ide-iops.c > >> +++ b/drivers/ide/ide-iops.c > [...] > > > What's interesting, ide_config_drive_speed() code looks sane in this > > respect: > > ... except one thing: > > >> @@ -769,13 +768,12 @@ int ide_config_drive_speed(ide_drive_t * > >> SELECT_DRIVE(drive); > >> SELECT_MASK(drive, 0); > > We've called disable_irq_nosync() before that, so it's not clear why we're > calling the driver's maskproc() method with 0 -- which unmasks interrupt in > the IDE chip. It seems to be an obvious bug (0 instead of 1) but it is hidden on almost all host drivers since they don't implement ->maskproc method (only icside, hpt366 and sgiioc4 do). [ disable_irq_nosync() is used due to other reasons than ->maskproc. The former is a band-aid for racing against IRQ handler, the latter is needed by icside to setup routing of IRQs and by hpt366 to handle ->quirk_list devices. ] > >> udelay(1); > >> - if (IDE_CONTROL_REG) > >> - hwif->OUTB(drive->ctl | 2, IDE_CONTROL_REG); > >> + ide_set_irq(drive, 0); > >> hwif->OUTB(speed, IDE_NSECTOR_REG); > >> hwif->OUTB(SETFEATURES_XFER, IDE_FEATURE_REG); > >> hwif->OUTBSYNC(drive, WIN_SETFEATURES, IDE_COMMAND_REG); > >> - if ((IDE_CONTROL_REG) && (drive->quirk_list == 2)) > >> - hwif->OUTB(drive->ctl, IDE_CONTROL_REG); > >> + if (drive->quirk_list == 2) > >> + ide_set_irq(drive, 1); > >> error = __ide_wait_stat(drive, drive->ready_stat, > >> BUSY_STAT|DRQ_STAT|ERR_STAT, > > Another SELECT_MASK(drive, 0) call follows which just doesn't make any > sense since the interrupt has been already unmasked by the first call. This one makes sense here if we assume that the previous one was buggy. Thanks, Bart -- 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
