ide: add ide_set_irq() inline helper

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 15 messages Nov 29, 2007 – Jul 11, 2008 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-11-29 00:02 UTC #0 lore raw

PATCH 2/8 ide: add ide_set_irq() inline helper

There should be no functionality changes caused by this patch.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/ide-io.c       |    9 +++------
 drivers/ide/ide-iops.c     |   10 ++++------
 drivers/ide/ide-probe.c    |   12 ++++--------
 drivers/ide/ide-taskfile.c |    3 +--
 include/linux/ide.h        |    5 +++++
 5 files changed, 17 insertions(+), 22 deletions(-)

drivers/ide/ide-io.c +3 -6

Index: b/drivers/ide/ide-io.c
===================================================================
@@ -939,8 +939,7 @@ static void ide_check_pm_state(ide_drive
 		if (rc)
 			printk(KERN_WARNING "%s: bus not ready on wakeup\n", drive->name);
 		SELECT_DRIVE(drive);
-		if (IDE_CONTROL_REG)
-			HWIF(drive)->OUTB(drive->ctl, IDE_CONTROL_REG);
+		ide_set_irq(drive, 1);
 		rc = ide_wait_not_busy(HWIF(drive), 100000);
 		if (rc)
 			printk(KERN_WARNING "%s: drive not ready on wakeup\n", drive->name);

@@ -1212,15 +1211,13 @@ static void ide_do_request (ide_hwgroup_
 		}
 	again:
 		hwif = HWIF(drive);
-		if (hwgroup->hwif->sharing_irq &&
-		    hwif != hwgroup->hwif &&
-		    hwif->io_ports[IDE_CONTROL_OFFSET]) {
+		if (hwgroup->hwif->sharing_irq && hwif != hwgroup->hwif) {
 			/*
 			 * set nIEN for previous hwif, drives in the
 			 * quirk_list may not like intr setups/cleanups
 			 */
 			if (drive->quirk_list != 1)
-				hwif->OUTB(drive->ctl | 2, IDE_CONTROL_REG);
+				ide_set_irq(drive, 0);
 		}
 		hwgroup->hwif = hwif;
 		hwgroup->drive = drive;

drivers/ide/ide-iops.c +4 -6

Index: 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);
 	msleep(50);
 	hwif->OUTB(WIN_IDENTIFY, IDE_COMMAND_REG);
 	timeout = jiffies + WAIT_WORSTCASE;

@@ -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);
 	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,

drivers/ide/ide-probe.c +4 -8

Index: b/drivers/ide/ide-probe.c
===================================================================
@@ -364,22 +364,19 @@ static int try_to_identify (ide_drive_t 
 	 * the irq handler isn't expecting.
 	 */
 	if (IDE_CONTROL_REG) {
-		u8 ctl = drive->ctl | 2;
 		if (!hwif->irq) {
 			autoprobe = 1;
 			cookie = probe_irq_on();
-			/* enable device irq */
-			ctl &= ~2;
 		}
-		hwif->OUTB(ctl, IDE_CONTROL_REG);
+		ide_set_irq(drive, autoprobe);
 	}
 
 	retval = actual_try_to_identify(drive, cmd);
 
 	if (autoprobe) {
 		int irq;
-		/* mask device irq */
-		hwif->OUTB(drive->ctl|2, IDE_CONTROL_REG);
+
+		ide_set_irq(drive, 0);
 		/* clear drive IRQ */
 		(void) hwif->INB(IDE_STATUS_REG);
 		udelay(5);

@@ -667,8 +664,7 @@ static int wait_hwif_ready(ide_hwif_t *h
 		/* Ignore disks that we will not probe for later. */
 		if (!drive->noprobe || drive->present) {
 			SELECT_DRIVE(drive);
-			if (IDE_CONTROL_REG)
-				hwif->OUTB(drive->ctl, IDE_CONTROL_REG);
+			ide_set_irq(drive, 1);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
 			if (rc)

drivers/ide/ide-taskfile.c +1 -2

Index: b/drivers/ide/ide-taskfile.c
===================================================================
@@ -83,8 +83,7 @@ void ide_tf_load(ide_drive_t *drive, ide
 		tf->hob_lbam, tf->hob_lbah);
 #endif
 
-	if (IDE_CONTROL_REG)
-		hwif->OUTB(drive->ctl, IDE_CONTROL_REG); /* clear nIEN */
+	ide_set_irq(drive, 1);
 
 	if ((task->tf_flags & IDE_TFLAG_NO_SELECT_MASK) == 0)
 		SELECT_MASK(drive, 0);

include/linux/ide.h +5 -0

Index: 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 */

Sergei Shtylyov

2008-07-07 15:47 UTC #1 lore raw

Hello.

Quoted 1 line · Bartlomiej Zolnierkiewicz

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

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?..

drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 691

Quoted 11 lines

Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +1 -2
@@ -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).

drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 692

Quoted 1 line

drivers/ide/ide-iops.c +0 -0
 	msleep(50);

Not sure why this substantial delay is needed at all...

drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 694

Quoted 2 lines

drivers/ide/ide-iops.c +0 -0

 	hwif->OUTB(WIN_IDENTIFY, IDE_COMMAND_REG);
 	timeout = jiffies + WAIT_WORSTCASE;

Here's the code that follows:

Quoted 1 line

        do {

The unexpected interrupt happens in this loop -- no wonder, we have no
hwgroup->handler installed.

Quoted 6 lines

                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"? :-)

Quoted 4 lines

                stat = ide_read_altstatus(drive);
        } while (stat & BUSY_STAT);
                                                                                
        msleep(50);     /* wait for IRQ and DRQ_STAT */

Again, isn't that too much?

Quoted 1 line

        stat = ide_read_status(drive);

This should clear the interrupt pending state on drive...

Quoted 6 lines

        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);

Quoted 8 lines

        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.

Quoted 2 lines

        local_irq_enable();
        local_irq_restore(flags);

What's interesting, ide_config_drive_speed() code looks sane in this respect:

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 771

Quoted 7 lines

drivers/ide/ide-iops.c +1 -2

@@ -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

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 776

Quoted 7 lines

drivers/ide/ide-iops.c +2 -2

 	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...

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 779

Quoted 2 lines

drivers/ide/ide-iops.c +0 -0

 	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:

Quoted 5 lines

Index: b/include/linux/ide.h
===================================================================

include/linux/ide.h static inline ide_drive_t *ide_get_paire · line 1310

Quoted 9 lines

include/linux/ide.h +5 -0

@@ -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

Sergei Shtylyov

2008-07-07 17:00 UTC #2 lore raw

Quoted 4 lines · Hello, I wrote:

Quoted 1 line
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
   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?..

Ah, I know why: unexpected_intr() just is never called for PCI chips.
Here's an related excerpt from ide_intr():

#ifdef CONFIG_BLK_DEV_IDEPCI
                if (hwif->chipset != ide_pci)
#endif  /* CONFIG_BLK_DEV_IDEPCI */
                {
                        /*
                         * Probably not a shared PCI interrupt,
                         * so we can safely try to do something about it:
                         */
                        unexpected_intr(irq, hwgroup);
#ifdef CONFIG_BLK_DEV_IDEPCI
                } else {
                        /*
                         * Whack the status register, just in case
                         * we have a leftover pending IRQ.
                         */
                        (void) hwif->INB(hwif->io_ports.status_addr);

#endif /* CONFIG_BLK_DEV_IDEPCI */

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2008-07-07 18:11 UTC #3 lore raw

Quoted 31 lines · Sergei Shtylyov

Quoted 9 lines · Hello, I wrote:
Quoted 1 line
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
   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?..
Ah, I know why: unexpected_intr() just is never called for PCI chips.
Here's an related excerpt from ide_intr():
#ifdef CONFIG_BLK_DEV_IDEPCI
                 if (hwif->chipset != ide_pci)
#endif  /* CONFIG_BLK_DEV_IDEPCI */
                 {
                         /*
                          * Probably not a shared PCI interrupt,
                          * so we can safely try to do something about it:
                          */
                         unexpected_intr(irq, hwgroup);
#ifdef CONFIG_BLK_DEV_IDEPCI
                 } else {
                         /*
                          * Whack the status register, just in case
                          * we have a leftover pending IRQ.
                          */
                         (void) hwif->INB(hwif->io_ports.status_addr);

Quoted 1 line

#endif /* CONFIG_BLK_DEV_IDEPCI */

Yes, we may consider removing #ifdef-s and always calling unexpected_intr() (ide_driveid_update() needs fixing first though).

Thanks,
Bart

Sergei Shtylyov

2008-07-08 09:13 UTC #4 lore raw

Quoted 24 lines · Bartlomiej Zolnierkiewicz

Quoted 22 lines
Quoted 2 lines
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?..
Ah, I know why: unexpected_intr() just is never called for PCI chips.
Here's an related excerpt from ide_intr():
#ifdef CONFIG_BLK_DEV_IDEPCI
                if (hwif->chipset != ide_pci)
#endif  /* CONFIG_BLK_DEV_IDEPCI */
                {
                        /*
                         * Probably not a shared PCI interrupt,
                         * so we can safely try to do something about it:
                         */
                        unexpected_intr(irq, hwgroup);
#ifdef CONFIG_BLK_DEV_IDEPCI
                } else {
                        /*
                         * Whack the status register, just in case
                         * we have a leftover pending IRQ.
                         */
                        (void) hwif->INB(hwif->io_ports.status_addr);
Quoted 1 line
#endif /* CONFIG_BLK_DEV_IDEPCI */

Quoted 2 lines

Yes, we may consider removing #ifdef-s and always calling unexpected_intr() (ide_driveid_update() needs fixing first though).

I think these changes are orthogonal, i.e. the message won't pop up on PCI
IDE chips whether #ifdef's are removed or not.

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2008-07-07 18:11 UTC #5 lore raw

In reply to Sergei Shtylyov

Hi,

Quoted 5 lines · Sergei Shtylyov

Hello.
Quoted 1 line · Bartlomiej Zolnierkiewicz
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Hmm, this patch already went in on Jan 26 (commit 81ca691981da718727281238b435dcf1528d2fda).

Quoted 7 lines

    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).

[ 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). ]

Quoted 7 lines

Quoted 7 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +0 -0
@@ -688,8 +688,7 @@ int ide_driveid_update(ide_drive_t *driv
 	 */
 

Quoted 10 lines

drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 691
Quoted 4 lines
drivers/ide/ide-iops.c +1 -2
 	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.

Quoted 3 lines

drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 692
Quoted 1 line
drivers/ide/ide-iops.c +0 -0
 	msleep(50);
Not sure why this substantial delay is needed at all...

Probably not neeed - this code was historically based on ide-probe.c so I guess it came from there.

Quoted 10 lines

drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 694
Quoted 2 lines
drivers/ide/ide-iops.c +0 -0
 	hwif->OUTB(WIN_IDENTIFY, IDE_COMMAND_REG);
 	timeout = jiffies + WAIT_WORSTCASE;
Here's the code that follows:
Quoted 1 line
        do {
The unexpected interrupt happens in this loop -- no wonder, we have no
hwgroup->handler installed.

Quoted 8 lines

Quoted 6 lines

                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"? :-)

ditto

Quoted 6 lines

Quoted 4 lines

                stat = ide_read_altstatus(drive);
        } while (stat & BUSY_STAT);
                                                                                
        msleep(50);     /* wait for IRQ and DRQ_STAT */

Again, isn't that too much?

ditto

Quoted 19 lines

Quoted 1 line

        stat = ide_read_status(drive);

This should clear the interrupt pending state on drive...

Quoted 6 lines

        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);

Quoted 8 lines

        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 */

Quoted 1 line

Too late, it's been surely cleared already.

Agreed.

Quoted 16 lines

Quoted 2 lines
        local_irq_enable();
        local_irq_restore(flags);
What's interesting, ide_config_drive_speed() code looks sane in this respect:
drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 771
Quoted 7 lines
drivers/ide/ide-iops.c +1 -2
@@ -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

Quoted 9 lines

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 776
Quoted 7 lines
drivers/ide/ide-iops.c +2 -2
 	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).

Since you have both the affected hardware and needed expertise I assume that you'll take care fixing ide_driveid_update()?

Thanks,
Bart

Sergei Shtylyov

2008-07-08 09:10 UTC #6 lore raw

Hello.

Quoted 4 lines · Bartlomiej Zolnierkiewicz

Quoted 1 line
Quoted 1 line
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Hmm, this patch already went in on Jan 26 (commit 81ca691981da718727281238b435dcf1528d2fda).

I know. :-)

Quoted 6 lines

Quoted 2 lines
   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. :-)

Quoted 4 lines

[ 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.

Quoted 15 lines

Quoted 7 lines
Quoted 7 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +0 -0
@@ -688,8 +688,7 @@ int ide_driveid_update(ide_drive_t *driv
 	 */
 
Quoted 7 lines
drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 691
Quoted 4 lines
drivers/ide/ide-iops.c +1 -2
 	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).

Quoted 1 line

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...

Quoted 5 lines

Quoted 4 lines
drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 693
Quoted 2 lines
drivers/ide/ide-iops.c +0 -0
       local_irq_enable();
       local_irq_restore(flags);
What's interesting, ide_config_drive_speed() code looks sane in this respect:

Quoted 10 lines

Quoted 10 lines
drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 771
Quoted 7 lines
drivers/ide/ide-iops.c +1 -2
@@ -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.

Quoted 11 lines

Quoted 9 lines
drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 776
Quoted 7 lines
drivers/ide/ide-iops.c +2 -2
 	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.

Quoted 2 lines

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...

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2008-07-10 21:25 UTC #7 lore raw

On Tuesday 08 July 2008, Sergei Shtylyov wrote:

[...]

Quoted 18 lines

Quoted 17 lines
Quoted 17 lines
drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 691
Quoted 11 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +1 -2
@@ -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).

Quoted 4 lines

Quoted 1 line
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...

I did some research on ->quirk_list in the past but I couldn't exactly figure it out. I also wasn't able to trace this code to its author (not that I tried very hard)...

Anyway my current findings/theories are the following:

- the diff between hpt366 and pdc202xx_{new,old} quirky devices:

pdc202xx_new.c 2008-04-12 22:04:03.000000000 +0200 renamed from hpt366.c 2008-04-12 22:03:54.000000000 +0200 +5 -1

@@ -1,7 +1,11 @@
-static const char *quirk_drives[] = {
+static const char *pdc_quirk_drives[] = {
        "QUANTUM FIREBALLlct08 08",
        "QUANTUM FIREBALLP KA6.4",
+       "QUANTUM FIREBALLP KA9.1",
        "QUANTUM FIREBALLP LM20.4",
+       "QUANTUM FIREBALLP KX13.6",
+       "QUANTUM FIREBALLP KX20.5",
+       "QUANTUM FIREBALLP KX27.3",
        "QUANTUM FIREBALLP LM20.5",
        NULL
 };

indicates that pdc202xx_{new,old} lists lack few devices that were added only to hpt366 and need fixing

- different values 1 (used by hpt366) or 2 (used by pdc202xx_new) are used stricly for historical reasons (IOW they are the result of people hitting problems with nIEN in different places and lack of generic solution) so we should make ->quirk_list a single bit flag and enable all quirks for both host drivers

- the problem most likely is not limited to hpt366/pdc202xx_{new,old} so we should move its handling to core IDE code

- we should make sure that all SELECT_MASK() users also call ide_set_irq() (then sgiioc4's ->maskproc can be removed since it is a copy of ->set_irq)

- we may want to make SELECT_MASK() call enable/disable_irq() for ->quirk_list device if there is no >maskproc method

- we need to be a bit careful with hpt366's ->maskproc because it checks for ->quirk_list internally (so either we need to mask/unmask for all devices on hpt366 or limit masking/unmasking to ->quirk_list on icside)

- we should merge SELECT_MASK() with ide_set_irq() and consider skipping ide_set_irq() for ->quirk_list devices

There is an added bonus in fixing the above issues as it would result in (finally) having sane API for IRQ (un)masking, namely:

* ->set_irq in struct ide_tp_ops for device's side

* ->maskproc in struct ide_port_ops for controller's side

As usual, I'll be glad to help with any patches.

Thanks,
Bart

Sergei Shtylyov

2008-07-11 09:28 UTC #8 lore raw

Hello.

Quoted 45 lines · Bartlomiej Zolnierkiewicz

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 692
Quoted 12 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +1 -2
@@ -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).
Quoted 5 lines
Quoted 2 lines
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...
I did some research on ->quirk_list in the past but I couldn't exactly figure it out. I also wasn't able to trace this code to its author (not that I tried very hard)...
Anyway my current findings/theories are the following:
- the diff between hpt366 and pdc202xx_{new,old} quirky devices:
pdc202xx_new.c 2008-04-12 22:04:03.000000000 +0200 renamed from hpt366.c 2008-04-12 22:03:54.000000000 +0200 +5 -1
@@ -1,7 +1,11 @@
-static const char *quirk_drives[] = {
+static const char *pdc_quirk_drives[] = {
        "QUANTUM FIREBALLlct08 08",
        "QUANTUM FIREBALLP KA6.4",
+       "QUANTUM FIREBALLP KA9.1",
        "QUANTUM FIREBALLP LM20.4",
+       "QUANTUM FIREBALLP KX13.6",
+       "QUANTUM FIREBALLP KX20.5",
+       "QUANTUM FIREBALLP KX27.3",
        "QUANTUM FIREBALLP LM20.5",
        NULL
 };

Quoted 3 lines

indicates that pdc202xx_{new,old} lists lack few devices that were added only to hpt366 and need fixing

You surely meant to say the hpt366's list lacks few devices. ;-)

Quoted 3 lines

- the problem most likely is not limited to hpt366/pdc202xx_{new,old} so we should move its handling to core IDE code

Agreed.

Quoted 3 lines

- we should make sure that all SELECT_MASK() users also call ide_set_irq() (then sgiioc4's ->maskproc can be removed since it is a copy of ->set_irq)

I've kind of made sure about that already, hence I've already created
a patch doing that...

Quoted 3 lines

- we may want to make SELECT_MASK() call enable/disable_irq() for ->quirk_list device if there is no >maskproc method

Agreed.

Quoted 2 lines

- we need to be a bit careful with hpt366's ->maskproc because it checks for ->quirk_list internally

... and falls back to manipuating nIEN if not -- which is not its
business.

Quoted 2 lines

(so either we need to mask/unmask for all devices on hpt366

I looked at that masking code and didn't like it at all -- since the
interrupts are disabled for *both* channels as there's only *one* bit controlling that. :-/ So, masking interrupt for all drives is not really desirable...

Quoted 2 lines

or limit masking/unmasking to ->quirk_list on icside)

But you just said that in the icside driver this code controls some
kind of IRQ routinng...

Quoted 3 lines

- we should merge SELECT_MASK() with ide_set_irq() and consider skipping ide_set_irq() for ->quirk_list devices

... you mean where it's not already done?

Quoted 9 lines

There is an added bonus in fixing the above issues as it would result in (finally) having sane API for IRQ (un)masking, namely:
* ->set_irq in struct ide_tp_ops for device's side
* ->maskproc in struct ide_port_ops for controller's side
As usual, I'll be glad to help with any patches.

Unfortunately, I'm very time-limited currently.

Quoted 3 lines

Thanks,
Bart

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2008-07-11 10:36 UTC #9 lore raw

Hi,

Quoted 52 lines · Sergei Shtylyov

Hello.
Quoted 48 lines · Bartlomiej Zolnierkiewicz
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 692
Quoted 12 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +1 -2
@@ -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).
Quoted 5 lines
Quoted 2 lines
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...
I did some research on ->quirk_list in the past but I couldn't exactly figure it out. I also wasn't able to trace this code to its author (not that I tried very hard)...
Anyway my current findings/theories are the following:
- the diff between hpt366 and pdc202xx_{new,old} quirky devices:
pdc202xx_new.c 2008-04-12 22:04:03.000000000 +0200 renamed from hpt366.c 2008-04-12 22:03:54.000000000 +0200 +5 -1
@@ -1,7 +1,11 @@
-static const char *quirk_drives[] = {
+static const char *pdc_quirk_drives[] = {
        "QUANTUM FIREBALLlct08 08",
        "QUANTUM FIREBALLP KA6.4",
+       "QUANTUM FIREBALLP KA9.1",
        "QUANTUM FIREBALLP LM20.4",
+       "QUANTUM FIREBALLP KX13.6",
+       "QUANTUM FIREBALLP KX20.5",
+       "QUANTUM FIREBALLP KX27.3",
        "QUANTUM FIREBALLP LM20.5",
        NULL
 };

Quoted 5 lines

Quoted 3 lines
indicates that pdc202xx_{new,old} lists lack few devices that were added only to hpt366 and need fixing
You surely meant to say the hpt366's list lacks few devices. ;-)

Yeah. :)

[...]

Quoted 6 lines

Quoted 2 lines
- we need to be a bit careful with hpt366's ->maskproc because it checks for ->quirk_list internally
... and falls back to manipuating nIEN if not -- which is not its
business.

Quoted 6 lines

Quoted 2 lines
(so either we need to mask/unmask for all devices on hpt366
I looked at that masking code and didn't like it at all -- since the
interrupts are disabled for *both* channels as there's only *one* bit controlling that. :-/

:-/ indeed, I didn't know about this.

Quoted 1 line

So, masking interrupt for all drives is not really desirable...

Agreed.

Quoted 5 lines

Quoted 2 lines
or limit masking/unmasking to ->quirk_list on icside)
But you just said that in the icside driver this code controls some
kind of IRQ routinng...

I later noticed that icside now also uses expansioncard_ops_t to do it (like pata_icside which doesn't have ->maskproc alike) so ->maskproc may no longer be needed for this.

Quoted 5 lines

Quoted 3 lines
- we should merge SELECT_MASK() with ide_set_irq() and consider skipping ide_set_irq() for ->quirk_list devices
... you mean where it's not already done?

We are going to mask the IRQ on controller side or enable/disable_irq() anyway for ->quirk_list devices so we setting nIEN doesn't seem necessary?

Thanks,
Bart

Sergei Shtylyov

2008-07-11 12:57 UTC #10 lore raw

In reply to Sergei Shtylyov

Quoted 24 lines · Hello, I wrote:

Quoted 23 lines
Quoted 15 lines
Quoted 14 lines
Quoted 14 lines
drivers/ide/ide-iops.c int ide_driveid_update(ide_drive_t *driv · line 691
Quoted 11 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +1 -2
@@ -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).
Quoted 4 lines
Quoted 1 line
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...
I did some research on ->quirk_list in the past but I couldn't exactly figure it out. I also wasn't able to trace this code to its author (not that I tried very hard)...

Quoted 6 lines

Quoted 4 lines
Anyway my current findings/theories are the following:
- we should make sure that all SELECT_MASK() users also call ide_set_irq() (then sgiioc4's ->maskproc can be removed since it is a copy of ->set_irq)
I've kind of made sure about that already,

After another look, I've indeed correctly described this as "kind of" --
since ide_set_irq() is called only before issuing a command, while
SELECT_MASK() is called both before (but it seems to be always skipped due to IDE_TFLAG_NO_SELECT_MASK being set for the commands executed in a normal way, i.e. using interrupts) and after that (the latter is done if we are using the polling mode). So, those maskproc() methods in hpt366 and sgiioc4 in fact inadvertently clear nIEN after the polled command execution -- not that it's wrong but why the hell? And those maskproc() methods duplicate nIEN setup before the command execution -- except in ide_config_drive_speed() where there's an obvious error...

Quoted 1 line

hence I've already created a patch doing that...

As I have no idea why sgiioc4 needs to manipulate nIEN, I guess I'll just
CC the SGI people on that patch...

Quoted 2 lines

Quoted 2 lines
Thanks,
Bart

MBR, Sergei

Sergei Shtylyov

2008-07-10 12:11 UTC #11 lore raw

Dubious IRQ masking in ide_config_drive_speed()

In reply to Sergei Shtylyov

Quoted 4 lines · Hello, I wrote:

Quoted 4 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +0 -0

[...]

Quoted 2 lines

What's interesting, ide_config_drive_speed() code looks sane in this
respect:

... except one thing:

Quoted 3 lines

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 769
Quoted 3 lines
drivers/ide/ide-iops.c +0 -0
@@ -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.

Quoted 5 lines

drivers/ide/ide-iops.c +1 -2

     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);

Quoted 8 lines

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 779

Quoted 8 lines

drivers/ide/ide-iops.c +2 -2

     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.

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2008-07-10 19:43 UTC #12 lore raw

Re: Dubious IRQ masking in ide_config_drive_speed()

Quoted 13 lines · Sergei Shtylyov

Quoted 4 lines · Hello, I wrote:
Quoted 4 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +0 -0
[...]
Quoted 2 lines
What's interesting, ide_config_drive_speed() code looks sane in this
respect:
... except one thing:

Quoted 7 lines

Quoted 3 lines
drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 769
Quoted 3 lines
drivers/ide/ide-iops.c +0 -0
@@ -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. ]

Quoted 14 lines

Quoted 5 lines

drivers/ide/ide-iops.c +1 -2

     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);

Quoted 8 lines

drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 779

Quoted 8 lines

drivers/ide/ide-iops.c +2 -2

     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,

Quoted 2 lines

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

Sergei Shtylyov

2008-07-10 21:21 UTC #13 lore raw

Re: Dubious IRQ masking in ide_config_drive_speed()

Hello.

Quoted 22 lines · Bartlomiej Zolnierkiewicz

Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
Index: b/drivers/ide/ide-iops.c
===================================================================
drivers/ide/ide-iops.c +0 -0
[...]
Quoted 3 lines
What's interesting, ide_config_drive_speed() code looks sane in this
respect:
... except one thing:
Quoted 7 lines
Quoted 4 lines
drivers/ide/ide-iops.c int ide_config_drive_speed(ide_drive_t * · line 770
Quoted 4 lines
drivers/ide/ide-iops.c +0 -0
@@ -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.

Quoted 4 lines

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).

Speaking of which, the hpt366 and sgiioc4 drivers try to manipulate
iIEN there which is none oif their business I think -- IDE core does that already. The patches are cooking. ;-)

Quoted 4 lines

[ disable_irq_nosync() is used due to other reasons than ->maskproc.
The former is a band-aid for racing against IRQ handler, the latter

Hm, but aren't we setting nIEN? Or could that cause a spuriuos interrupt?

Quoted 1 line

is needed by icside to setup routing of IRQs

Hm, that's what I asn't able to figure out gazing at it...

Quoted 3 lines

and by hpt366 to handle
->quirk_list devices. ]

BTW, I was counting on your feedback concerning driver->quirk_list
handling.
Do you think the current patch for ide_driveid_update() is acceptable?

Quoted 14 lines

drivers/ide/ide-iops.c +3 -4

     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,

Quoted 6 lines

Quoted 3 lines
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.

Of cousre. :-)

Quoted 3 lines

Thanks,
Bart

MBR, Sergei

Bartlomiej Zolnierkiewicz

author

2008-07-10 21:49 UTC #14 lore raw

Re: Dubious IRQ masking in ide_config_drive_speed()

On Thursday 10 July 2008, you wrote:

[...]

Quoted 2 lines

BTW, I was counting on your feedback concerning driver->quirk_list
handling.

Done, please see the other mail.

Sorry for the delay, I needed to find some "straight" time to calmly recall/recheck/rethink all issues related to ->quirk_list.

Quoted 1 line

Do you think the current patch for ide_driveid_update() is acceptable?

I applied it, didn't I? :)

Thanks,
Bart