Re: possibility to merge via_pata fixes to .19 stable series?

Re: possibility to merge via_pata fixes to .19 stable series?

[Jeff Garzik]

Andrew Morton wrote:
> On Sat, 18 Nov 2006 07:59:27 +0100
> Tobias Powalowski <t.powa@gmx.de> wrote:
> 
>> the latest .19rc6 still didn't inlcude your 2 patches for via_pata patches 
>> from -mm kernel.
>> They are at least needed for my pc to boot fine, so i guess others would be 
>> also happy if those were upstream applied, else i have to patch our final 
>> kernel then.
> 
> If you check the full changelog
> (ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19-rc5/2.6.19-rc5-mm2/broken-out/via-pata-controller-xfer-fixes.patch)
> you'll see that Jeff had issues with the patch.
> 
> I don't know if they're still concerns, nor if they're fatal concerns
> either.  But I've been sitting on that patch for nearly five months.

This isn't driver-specific, so a similar change should be applied to the 
core.

	Jeff

Re: possibility to merge via_pata fixes to .19 stable series?

[Alan]

On Sat, 18 Nov 2006 16:20:12 -0500
Jeff Garzik <jeff@garzik.org> wrote:
> > I don't know if they're still concerns, nor if they're fatal concerns
> > either.  But I've been sitting on that patch for nearly five months.
> 
> This isn't driver-specific, so a similar change should be applied to the 
> core.

Agreed - it belongs in the core code. We see the same problem on multiple
controllers and its one of the big things that at the moment makes it
impossible to progress the PATA drivers further.

Re: possibility to merge via_pata fixes to .19 stable series?

[Andrew Morton]

On Sat, 18 Nov 2006 22:43:34 +0000
Alan <alan@lxorguk.ukuu.org.uk> wrote:

> On Sat, 18 Nov 2006 16:20:12 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
> > > I don't know if they're still concerns, nor if they're fatal concerns
> > > either.  But I've been sitting on that patch for nearly five months.
> > 
> > This isn't driver-specific, so a similar change should be applied to the 
> > core.
> 
> Agreed - it belongs in the core code. We see the same problem on multiple
> controllers and its one of the big things that at the moment makes it
> impossible to progress the PATA drivers further.

A five month stall of a patch which makes the difference betweem "working"
and "not working" sounds pretty serious.

Is this something someone is working on?  If not, is it something which
Matthieu might have time to look into (please?)

Re: possibility to merge via_pata fixes to .19 stable series?

[Tejun Heo]

Andrew Morton wrote:
> On Sat, 18 Nov 2006 22:43:34 +0000
> Alan <alan@lxorguk.ukuu.org.uk> wrote:
> 
>> On Sat, 18 Nov 2006 16:20:12 -0500
>> Jeff Garzik <jeff@garzik.org> wrote:
>>>> I don't know if they're still concerns, nor if they're fatal concerns
>>>> either.  But I've been sitting on that patch for nearly five months.
>>> This isn't driver-specific, so a similar change should be applied to the 
>>> core.
>> Agreed - it belongs in the core code. We see the same problem on multiple
>> controllers and its one of the big things that at the moment makes it
>> impossible to progress the PATA drivers further.
> 
> A five month stall of a patch which makes the difference betweem "working"
> and "not working" sounds pretty serious.
> 
> Is this something someone is working on?  If not, is it something which
> Matthieu might have time to look into (please?)

Doing SETXFER via polling is proposed as solution and verfivied by Matthieu.

http://thread.gmane.org/gmane.linux.ide/13248/focus=13248

The patch has been updated and re-submitted a few days ago.

http://thread.gmane.org/gmane.linux.ide/14088/focus=14088

-- 
tejun

Re: possibility to merge via_pata fixes to .19 stable series?

[Andrew Morton]

On Sun, 19 Nov 2006 14:34:52 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Andrew Morton wrote:
> > On Sat, 18 Nov 2006 22:43:34 +0000
> > Alan <alan@lxorguk.ukuu.org.uk> wrote:
> > 
> >> On Sat, 18 Nov 2006 16:20:12 -0500
> >> Jeff Garzik <jeff@garzik.org> wrote:
> >>>> I don't know if they're still concerns, nor if they're fatal concerns
> >>>> either.  But I've been sitting on that patch for nearly five months.
> >>> This isn't driver-specific, so a similar change should be applied to the 
> >>> core.
> >> Agreed - it belongs in the core code. We see the same problem on multiple
> >> controllers and its one of the big things that at the moment makes it
> >> impossible to progress the PATA drivers further.
> > 
> > A five month stall of a patch which makes the difference betweem "working"
> > and "not working" sounds pretty serious.
> > 
> > Is this something someone is working on?  If not, is it something which
> > Matthieu might have time to look into (please?)
> 
> Doing SETXFER via polling is proposed as solution and verfivied by Matthieu.
> 
> http://thread.gmane.org/gmane.linux.ide/13248/focus=13248

Great, thanks.

> The patch has been updated and re-submitted a few days ago.
> 
> http://thread.gmane.org/gmane.linux.ide/14088/focus=14088
> 

OIC.  I'm not on linux-ide.

[PATCH] libata: waits up to 10 microseconds for early irq problem

[Albert Lee]

Some devices raise irq early before clearing the BSY.
This patch waits up to 10 microseconds to workaround the early irq problem.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
In additional to the pata_via SET FEATURES XFER problem reported by Matthieu,
we also see the early irq problem with ICH4 + AOpen CD-936E on READ under PIO mode 4.

The following is a port of the pata_via_fix to the libata core.
Patch against 2.6.19-rc6. For your review, thanks.


--- linux-2.6.19-rc6/include/linux/libata.h	2006-11-28 11:14:52.000000000 +0800
+++ linux-2.6.19-rc6-early_irq/include/linux/libata.h	2006-11-28 13:31:07.565194648 +0800
@@ -1039,6 +1039,30 @@ static inline void ata_pause(struct ata_
 	ndelay(400);
 }
 
+/**
+ *	ata_busy_wait_alt - Wait for a port alt status register
+ *	@ap: Port to wait for.
+ *
+ *	Waits up to max microseconds for the selected bits in the port's
+ *	alt status register to be cleared.
+ *	Returns final value of alt status register.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+static inline u8 ata_busy_wait_alt(struct ata_port *ap, unsigned int bits,
+				   unsigned int max)
+{
+	u8 status = ata_altstatus(ap);
+
+	while ((status & bits) && (max > 0)) {
+		udelay(1);
+		status = ata_altstatus(ap);
+		max--;
+	};
+
+	return status;
+}
 
 /**
  *	ata_busy_wait - Wait for a port status register
--- linux-2.6.19-rc6/drivers/ata/libata-core.c	2006-11-28 11:14:38.000000000 +0800
+++ linux-2.6.19-rc6-early_irq/drivers/ata/libata-core.c	2006-11-28 13:31:31.275590120 +0800
@@ -4828,8 +4828,10 @@ inline unsigned int ata_host_intr (struc
 		goto idle_irq;
 	}
 
-	/* check altstatus */
-	status = ata_altstatus(ap);
+	/* check altstatus.
+	 * Waits up to 10 microseconds for early irq.
+	 */
+	status = ata_busy_wait_alt(ap, ATA_BUSY, 10);
 	if (status & ATA_BUSY)
 		goto idle_irq;
 

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Alan]

On Tue, 28 Nov 2006 14:52:28 +0800
Albert Lee <albertcc@tw.ibm.com> wrote:

> Some devices raise irq early before clearing the BSY.
> This patch waits up to 10 microseconds to workaround the early irq problem.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Acked-by: Alan Cox <alan@redhat.com>

The IRQ delivery is async to the I/O so this makes a lot of sense for all
cases.

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Tejun Heo]

Alan wrote:
> On Tue, 28 Nov 2006 14:52:28 +0800
> Albert Lee <albertcc@tw.ibm.com> wrote:
> 
>> Some devices raise irq early before clearing the BSY.
>> This patch waits up to 10 microseconds to workaround the early irq problem.
>>
>> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Does this early IRQ happen for any command other than SET XFERMODE? 
Generic fix for SET XFERMODE is in the tree now.

> Acked-by: Alan Cox <alan@redhat.com>
> 
> The IRQ delivery is async to the I/O so this makes a lot of sense for all
> cases.

I don't think that's true unless the controller is doing something funky 
as in SET XFERMODE.  Can you enlighten me?

-- 
tejun

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Alan]

> > The IRQ delivery is async to the I/O so this makes a lot of sense for all
> > cases.
> 
> I don't think that's true unless the controller is doing something funky 
> as in SET XFERMODE.  Can you enlighten me?

It is true for all cases. There is no synchronization between interrupt
delivery and I/O cycles and both of them are asynchronous. It is
especially obvious on older APIC based boxes that use a 4 wire bus to
send interrupt messages around.

This leads to suprising sequences like


	device raises IRQ
				kernel blocks device IRQ at chip
				kernel reads to post the block
				kernel does other stuff
	IRQ message finally arrives
				IRQ taken

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Mark Lord]

Alan wrote:
>>> The IRQ delivery is async to the I/O so this makes a lot of sense for all
>>> cases.
>> I don't think that's true unless the controller is doing something funky 
>> as in SET XFERMODE.  Can you enlighten me?

I'm with Tejun on this one.  The posted patch simply adds extra bus transactions
to poll for a late BUSY bit.  Yes, some devices are late posting the BUSY bit,
but the vast majority are not.

For other cases where this is needed, I'm not so sure.
Alan, I confess that I really don't understand this example (below).
Can you try again, with something concrete enough for a rusty old
IDE hacker to understand?

Thanks

> This leads to suprising sequences like
> 
> 
> 	device raises IRQ
> 				kernel blocks device IRQ at chip
> 				kernel reads to post the block
> 				kernel does other stuff
> 	IRQ message finally arrives
> 				IRQ taken


-- 
Mark Lord
Real-Time Remedies Inc.
mlord@pobox.com

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Alan]

On Tue, 28 Nov 2006 09:00:44 -0500
Mark Lord <mlord@pobox.com> wrote:

> Alan, I confess that I really don't understand this example (below).
> Can you try again, with something concrete enough for a rusty old
> IDE hacker to understand?

The asynchronocity isn't at the IDE level - its at the PCI bus level. The
IRQ path between a PCI card and the CPU is not synchronous to I/O cycles.

Alan

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Mark Lord]

Alan wrote:
> On Tue, 28 Nov 2006 09:00:44 -0500
> Mark Lord <mlord@pobox.com> wrote:
> 
>> Alan, I confess that I really don't understand this example (below).
>> Can you try again, with something concrete enough for a rusty old
>> IDE hacker to understand?
> 
> The asynchronocity isn't at the IDE level - its at the PCI bus level. The
> IRQ path between a PCI card and the CPU is not synchronous to I/O cycles.

Yes, I know that much already.

But how does that factor into the need for this particular patch,
(apart from truly buggy drives which send the IRQ before clearing BUSY) ?

The device driver won't do ANY I/O until it sees the IRQ,
so things are synchronized already without needing to do
an extra status poll in response to the IRQ.

???

Thanks

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Albert Lee]

Tejun Heo wrote:
> 
> 
> Does this early IRQ happen for any command other than SET XFERMODE?
> Generic fix for SET XFERMODE is in the tree now.
> 

Unicorn found his ICH4 + AOpen CD-936E + PIO4 has the early irq problem
with PACKET command READ_10. (The CD-ROM drive is connected to the secondary
channel on legacy irq 15.) Detailed log attched below.

We replaced the AOpen drive with another LITEON drive, and the problem is
no longer reproducible. It looks like related to the specific CD-ROM drive.

--
albert

======
(1. early irq case)
Nov 24 15:05:39 albertnv kernel: CDB (2:0,0,0) 28 00 00 00 00 ed 00 00 01
Nov 24 15:05:39 albertnv kernel: feat 0x0 nsect 0x0 lba 0x0 0x0 0x20
Nov 24 15:05:39 albertnv kernel: device 0xA0
Nov 24 15:05:39 albertnv kernel: ata2: cmd 0xA0
Nov 24 15:05:39 albertnv kernel: ata2: protocol 5 task_state 5
Nov 24 15:05:39 albertnv kernel: ata2: protocol 5 task_state 5 (dev_stat 0x58)
Nov 24 15:05:39 albertnv kernel: atapi_send_cdb: send cdb
Nov 24 15:05:39 albertnv kernel: ata2: protocol 5 task_state 2
Nov 24 15:05:39 albertnv last message repeated 6 times    <=== early irq here
Nov 24 15:05:39 albertnv kernel: ata2: protocol 5 task_state 2 (dev_stat 0x58)
Nov 24 15:05:39 albertnv kernel: atapi_pio_bytes: ata2: xfering 2048 bytes
Nov 24 15:05:39 albertnv kernel: __atapi_pio_bytes: data read
Nov 24 15:05:39 albertnv kernel: ata2: protocol 5 task_state 3
Nov 24 15:05:39 albertnv kernel: ata2: protocol 5 task_state 3 (dev_stat 0x50)
Nov 24 15:05:39 albertnv kernel: ata2: dev 0 command complete, drv_stat 0x50
Nov 24 15:05:39 albertnv kernel: atapi_qc_complete: ENTER, err_mask 0x0

(2. normal case)
Nov 24 15:05:40 albertnv kernel: CDB (2:0,0,0) 28 00 00 00 00 ee 00 00 01
Nov 24 15:05:40 albertnv kernel: feat 0x0 nsect 0x0 lba 0x0 0x0 0x20
Nov 24 15:05:40 albertnv kernel: device 0xA0
Nov 24 15:05:40 albertnv kernel: ata2: cmd 0xA0
Nov 24 15:05:40 albertnv kernel: ata2: protocol 5 task_state 5
Nov 24 15:05:40 albertnv kernel: ata2: protocol 5 task_state 5 (dev_stat 0x58)
Nov 24 15:05:40 albertnv kernel: atapi_send_cdb: send cdb
Nov 24 15:05:40 albertnv kernel: ata2: protocol 5 task_state 2
Nov 24 15:05:40 albertnv kernel: ata2: protocol 5 task_state 2 (dev_stat 0x58)
Nov 24 15:05:40 albertnv kernel: atapi_pio_bytes: ata2: xfering 2048 bytes
Nov 24 15:05:40 albertnv kernel: __atapi_pio_bytes: data read
Nov 24 15:05:40 albertnv kernel: ata2: protocol 5 task_state 3
Nov 24 15:05:40 albertnv kernel: ata2: protocol 5 task_state 3 (dev_stat 0x50)
Nov 24 15:05:40 albertnv kernel: ata2: dev 0 command complete, drv_stat 0x50
Nov 24 15:05:40 albertnv kernel: atapi_qc_complete: ENTER, err_mask 0x0

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Albert Lee]

Albert Lee wrote:

> Some devices raise irq early before clearing the BSY.

> This patch waits up to 10 microseconds to workaround the early irq problem.

> 



BTW, the patch is on the hot path. The following is a quick analysis of
the performance penalty introduced by the patch:



1. DMA mode:

   The dma_status is checked before reading the alt_status, so we are pretty

   sure that it is our interrupt. It's worthwhile to poll alt_status and wait

   for BSY to clear in this case. 



2. PIO mode with non-shared irq:

   It is our interrupt. It's worthwhile to wait a moment for BSY to clear.



3. PIO mode with shared-irq:

   If PIO + shared-irq + qc active + other people's irq

   => We waste 10 micro seconds to figure out that it is not our interrupt.
      In exchange for the wasted time, we get better tolerance for the
      early irq situations.

--
albert

  

     

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Mark Lord]

Albert Lee wrote:
> It's worthwhile to poll alt_status and wait for BSY to clear in this case. 

Polling the ATA ALT_STATUS register can take up to 600ns,
or the equivalent of several thousand wasted instructions
on a modern CPU.

It's only worthwhile if the poll was actually necessary.
My question is, do we really want this overhead in the hot path
on every single ATA interrupt, or should we instead simply blacklist
those devices with the problem?

Or perhaps be more clever in handling it:  If the device driver interrupt
routine see's BUSY still set after the normal ATA STATUS read,
*then* we could do the poll.  That way it never adds unnecessary overhead.

This is more than "just a single instruction" here.. it's the equivalent
of thousands of them, each time we read an ATA register.

Cheers

Re: [PATCH] libata: waits up to 10 microseconds for early irq problem

[Jeff Garzik]

Mark Lord wrote:
> It's only worthwhile if the poll was actually necessary.
> My question is, do we really want this overhead in the hot path
> on every single ATA interrupt, or should we instead simply blacklist
> those devices with the problem?


Nod...  that's my thinking as well

	Jeff

[PATCH] libata: blacklist for early irq problem

[Albert Lee]

Some devices raise irq early before clearing the BSY.
This patch adds blacklist and waits up to 10 microseconds to workaround the early irq problem.

Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
---
Ok, we should not affect the good devices.
Patch revised for your review.
(against the libata-dev tree 6b78c0d20ffbc89acc3c2790f8d7eded05606813).

diff -Nrup 000_libata_dev/drivers/ata/libata-core.c 001_early_irq/drivers/ata/libata-core.c
--- 000_libata_dev/drivers/ata/libata-core.c	2006-11-30 15:53:08.000000000 +0800
+++ 001_early_irq/drivers/ata/libata-core.c	2006-11-30 16:41:23.000000000 +0800
@@ -1610,6 +1610,9 @@ int ata_dev_configure(struct ata_device 
 	if (dev->flags & ATA_DFLAG_LBA48)
 		dev->max_sectors = ATA_MAX_SECTORS_LBA48;
 
+	if (ata_device_blacklisted(dev) & ATA_HORKAGE_EARLY_IRQ)
+		dev->horkage |= ATA_HORKAGE_EARLY_IRQ;
+
 	if (dev->horkage & ATA_HORKAGE_DIAGNOSTIC) {
 		/* Let the user know. We don't want to disallow opens for
 		   rescue purposes, or in case the vendor is just a blithering
@@ -3184,6 +3187,9 @@ static const struct ata_blacklist_entry 
 
 	/* Devices with NCQ limits */
 
+	/* Devices with early IRQ */
+	{ "CD-ROM 36X/AKW",   NULL,		ATA_HORKAGE_EARLY_IRQ },
+
 	/* End Marker */
 	{ }
 };
@@ -4989,6 +4995,11 @@ inline unsigned int ata_host_intr (struc
 		goto idle_irq;
 	}
 
+	/* some drives raise INTRQ early before clearing BSY */
+	if (unlikely(qc->dev->horkage & ATA_HORKAGE_EARLY_IRQ))
+		/* wait up to 10 microseconds for BSY to clear */
+		ata_busy_wait_alt(ap, ATA_BUSY, ATA_EARLY_IRQ_WAIT);
+
 	/* check altstatus */
 	status = ata_altstatus(ap);
 	if (status & ATA_BUSY)
diff -Nrup 000_libata_dev/include/linux/libata.h 001_early_irq/include/linux/libata.h
--- 000_libata_dev/include/linux/libata.h	2006-11-30 15:53:26.000000000 +0800
+++ 001_early_irq/include/linux/libata.h	2006-11-30 17:04:37.000000000 +0800
@@ -309,6 +309,9 @@ enum {
 	 * most devices.
 	 */
 	ATA_SPINUP_WAIT		= 8000,
+
+	/*  early irq max wait time (for BSY to clear) in usecs */
+	ATA_EARLY_IRQ_WAIT	= 10,
 	
 	/* Horkage types. May be set by libata or controller on drives
 	   (some horkage may be drive/controller pair dependant */
@@ -316,6 +319,7 @@ enum {
 	ATA_HORKAGE_DIAGNOSTIC	= (1 << 0),	/* Failed boot diag */
 	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
+	ATA_HORKAGE_EARLY_IRQ	= (1 << 3),	/* Early IRQ */
 };
 
 enum hsm_task_states {
@@ -1056,6 +1060,30 @@ static inline void ata_pause(struct ata_
 	ndelay(400);
 }
 
+/**
+ *	ata_busy_wait_alt - Wait for a port alt status register
+ *	@ap: Port to wait for.
+ *
+ *	Waits up to max microseconds for the selected bits in the port's
+ *	alt status register to be cleared.
+ *	Returns final value of alt status register.
+ *
+ *	LOCKING:
+ *	Inherited from caller.
+ */
+static inline u8 ata_busy_wait_alt(struct ata_port *ap, unsigned int bits,
+				   unsigned int max)
+{
+	u8 status = ata_altstatus(ap);
+
+	while ((status & bits) && (max > 0)) {
+		udelay(1);
+		status = ata_altstatus(ap);
+		max--;
+	};
+
+	return status;
+}
 
 /**
  *	ata_busy_wait - Wait for a port status register

Re: [PATCH] libata: blacklist for early irq problem

[Alan]

On Thu, 30 Nov 2006 17:43:51 +0800
Albert Lee <albertcc@tw.ibm.com> wrote:

> Some devices raise irq early before clearing the BSY.
> This patch adds blacklist and waits up to 10 microseconds to workaround the early irq problem.
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>

Acked-by: Alan Cox <alan@redhat.com>

Looks good to me

Re: [PATCH] libata: blacklist for early irq problem

[Mark Lord]

Albert Lee wrote:
>..
> +	/* Devices with early IRQ */
> +	{ "CD-ROM 36X/AKW",   NULL,		ATA_HORKAGE_EARLY_IRQ },

Ahhh.. the 36X/AKW drive!  I should have known.
This one has caused all kinds of grief in the past for me,
though I no longer remember the exact details of it.

My two drives of this model finally stopped working (mechanically)
earlier this year, and got binned.  Good riddance!

Cheers

Re: [PATCH] libata: blacklist for early irq problem

[Mark Lord]

Albert Lee wrote:
>...
>  
> +	/* some drives raise INTRQ early before clearing BSY */
> +	if (unlikely(qc->dev->horkage & ATA_HORKAGE_EARLY_IRQ))
> +		/* wait up to 10 microseconds for BSY to clear */
> +		ata_busy_wait_alt(ap, ATA_BUSY, ATA_EARLY_IRQ_WAIT);
> +
>  	/* check altstatus */
>  	status = ata_altstatus(ap);
>  	if (status & ATA_BUSY)
...

I dunno.

I think a much simpler patch could address the same situation,
with *no blacklist* to maintain, and no added overhead required:

	/* check altstatus */
+	retries = 10;
+	while (((status = ata_altstatus(ap)) & ATA_BUSY) && --retries) {
+		udelay(1);
+	}
-	status = ata_altstatus(ap);
	if (status & ATA_BUSY)

This still just does a single access in the hot path, but without any
real added complexity otherwise.

????