Hello. Bartlomiej Zolnierkiewicz wrote:
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
Hmm, this patch already went in on Jan 26 (commit 81ca691981da718727281238b435dcf1528d2fda).
I know. :-)
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?..
You are right, I've also noticed this old bug while working on the patch but I didn't have time to deal with it yet (it was kind of low-prio since it doesn't affect IDE PCI controllers and has been there for a long time).
Really long time. :-)
[ BTW I would _strongly_ prefer to discuss the old bugs under new threads instead of replying to unrelated patches which just happen to be touching the buggy code (like my patch which didn't cause any funtionality changes and was just a preparation for adding struct ide_tp_ops). ]
Sorry about it. I'm doing all things in a hurry now... should have at least changed the subject.
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).
Yes, this needs fixing.
But should we honor drive->quirk_list here? What its different values mean? I'm seeing either 1 or 2 is used to decide whether to set nIEN or not...
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
Or not -- since nIEN can be later cleared.
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...
Agreed (however deserves a separate patch just in case we're wrong).
Of course.
Since you have both the affected hardware and needed expertise I assume that you'll take care fixing ide_driveid_update()?
Mikhail will submit the fix, and we'll see about cleanups later...
Thanks, Bart
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
