Re: Dubious IRQ masking in ide_config_drive_speed()

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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