Hello.
Bartlomiej Zolnierkiewicz wrote:
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
We're getting "ide0: unexpected interrupt, status=0x58, count=1" with
palm_bk3710 driver when running hdparm with option -X. That interrupt has
beenidentidied to occur while ide_driveid_update() waits for non-BSY status
polling the alt. status reg. After looking at the code there, I couldn't help
wondering why I never saw that before with any other controller since the code
looked like it was bound to produce the unexpected interrupts -- unless I'm
missing something?..
Index: b/drivers/ide/ide-iops.c
===================================================================
--- a/drivers/ide/ide-iops.c
+++ b/drivers/ide/ide-iops.c
@@ -688,8 +688,7 @@ int ide_driveid_update(ide_drive_t *driv
*/
SELECT_MASK(drive, 1);
- if (IDE_CONTROL_REG)
- hwif->OUTB(drive->ctl,IDE_CONTROL_REG);
+ ide_set_irq(drive, 1);
If we're going to execute the command using polling, isn't it logical to
*disable* drive's interrupt instead of enabling it which this code is
currently doing? This looks like it might work only for the drivers having
the maskproc() method (of which hpt366.c is the only one that I've ever dealt
with).
msleep(50);
Not sure why this substantial delay is needed at all...
hwif->OUTB(WIN_IDENTIFY, IDE_COMMAND_REG);
timeout = jiffies + WAIT_WORSTCASE;
Here's the code that follows:
do {
The unexpected interrupt happens in this loop -- no wonder, we have no
hwgroup->handler installed.
if (time_after(jiffies, timeout)) {
SELECT_MASK(drive, 0);
return 0; /* drive timed-out */
}
msleep(50); /* give drive a breather */
Isn't that too much for "a breather"? :-)
stat = ide_read_altstatus(drive);
} while (stat & BUSY_STAT);
msleep(50); /* wait for IRQ and DRQ_STAT */
Again, isn't that too much?
stat = ide_read_status(drive);
This should clear the interrupt pending state on drive...
if (!OK_STAT(stat, DRQ_STAT, BAD_R_STAT)) {
SELECT_MASK(drive, 0);
printk("%s: CHECK for good STATUS\n", drive->name);
return 0;
}
local_irq_save(flags);
SELECT_MASK(drive, 0);
id = kmalloc(SECTOR_WORDS*4, GFP_ATOMIC);
if (!id) {
local_irq_restore(flags);
return 0;
}
hwif->input_data(drive, NULL, id, SECTOR_SIZE);
(void)ide_read_status(drive); /* clear drive IRQ */
Too late, it's been surely cleared already.
local_irq_enable();
local_irq_restore(flags);
What's interesting, ide_config_drive_speed() code looks sane in this respect:
@@ -769,13 +768,12 @@ int ide_config_drive_speed(ide_drive_t *
SELECT_DRIVE(drive);
SELECT_MASK(drive, 0);
udelay(1);
- if (IDE_CONTROL_REG)
- hwif->OUTB(drive->ctl | 2, IDE_CONTROL_REG);
+ ide_set_irq(drive, 0);
This correctly sets nIEN... The code als calls disable_irq_nosync() which
might be an overkill
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);
I'm just not sure why set nIEN on the quirky drives at all...
error = __ide_wait_stat(drive, drive->ready_stat,
BUSY_STAT|DRQ_STAT|ERR_STAT,
Here's the code of ide_set_irq() for the reference:
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -1302,4 +1302,9 @@ static inline ide_drive_t *ide_get_paire
return &hwif->drives[(drive->dn ^ 1) & 1];
}
+static inline void ide_set_irq(ide_drive_t *drive, int on)
+{
+ drive->hwif->OUTB(drive->ctl | (on ? 0 : 2), IDE_CONTROL_REG);
+}
+
#endif /* _IDE_H */
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
