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

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

[Bartlomiej Zolnierkiewicz]

There should be no functionality changes caused by this patch.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 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(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ 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;
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);
 	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,
Index: b/drivers/ide/ide-probe.c
===================================================================
--- a/drivers/ide/ide-probe.c
+++ 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)
Index: b/drivers/ide/ide-taskfile.c
===================================================================
--- a/drivers/ide/ide-taskfile.c
+++ 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);
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 */

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

    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

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

[Sergei Shtylyov]

Hello, I wrote:

>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

>    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

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Monday 07 July 2008, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

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

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

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

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

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

ditto

> >                 stat = ide_read_altstatus(drive);
> >         } while (stat & BUSY_STAT);
> >                                                                                 
> >         msleep(50);     /* wait for IRQ and DRQ_STAT */
> 
>     Again, isn't that too much?

ditto

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

Agreed.

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

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

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

[Bartlomiej Zolnierkiewicz]

On Monday 07 July 2008, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> >    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 */

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

Thanks,
Bart

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

> 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

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

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

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

> Thanks,
> Bart

MBR, Sergei

Dubious IRQ masking in ide_config_drive_speed()

[Sergei Shtylyov]

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.

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

MBR, Sergei

Re: Dubious IRQ masking in ide_config_drive_speed()

[Sergei Shtylyov]

Hello.

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

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

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

>   is needed by icside to setup routing of IRQs

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

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

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

   Of cousre. :-)

> Thanks,
> Bart
>   

MBR, Sergei

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

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>>> 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...
>>     
>
> 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:
>
> --- hpt366.c    2008-04-12 22:03:54.000000000 +0200
> +++ pdc202xx_new.c      2008-04-12 22:04:03.000000000 +0200
> @@ -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
>   

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

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

   Agreed.

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

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

   Agreed.

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

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

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

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

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

> Thanks,
> Bart
>   

MBR, Sergei

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

[Sergei Shtylyov]

Hello, I wrote:

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

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

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

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

>> Thanks,
>> Bart

MBR, Sergei

Re: Dubious IRQ masking in ide_config_drive_speed()

[Bartlomiej Zolnierkiewicz]

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

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

[Bartlomiej Zolnierkiewicz]

On Tuesday 08 July 2008, Sergei Shtylyov wrote:

[...]

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

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:

--- hpt366.c    2008-04-12 22:03:54.000000000 +0200
+++ pdc202xx_new.c      2008-04-12 22:04:03.000000000 +0200
@@ -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

Re: Dubious IRQ masking in ide_config_drive_speed()

[Bartlomiej Zolnierkiewicz]

On Thursday 10 July 2008, you wrote:

[...]

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

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

I applied it, didn't I? :)

Thanks,
Bart

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

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday 11 July 2008, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>>> 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...
> >>     
> >
> > 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:
> >
> > --- hpt366.c    2008-04-12 22:03:54.000000000 +0200
> > +++ pdc202xx_new.c      2008-04-12 22:04:03.000000000 +0200
> > @@ -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
> >   
> 
>    You surely meant to say the hpt366's list lacks few devices. ;-)

Yeah. :)

[...]

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

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

Agreed.

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

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