On Tue, 05 Jun 2007 17:14:46 +0100, David Greaves <[email protected]> wrote:
>[Tejun, Jeff, added you since the bisect points to your patch.]
>
>Sorry, mail glitch means I lost a couple of emails...
>
>I said:
>Compile warnings and a new regression: hang on boot during sata_promise
>detection...
>
>It turns out that the hang times out; it does boot after a while. It's missing 4
>of my SATA disks though.
>[in turn this means I can't test the hibernate regression against -rc4. But
>testing that regression against a862b5c8cd5d847779a049a5fc8cf5b1e6f5fa07 shows
>it is still there. Do I get a bonus for finding 2 regressions?]]
>
>I also bisected and got:
>Bisecting: 0 revisions left to test after this
>[464cf177df7727efcc5506322fc5d0c8b896f545] libata: always use polling SETXFER
>
>According to marc, Mikail said:
That's Mikael, not Mikail.
>Please give us some details about your sata_promise problem:
>- describe your hardware (Promise chip version, mainboard, chipset, etc)
>I have a Promise TX-4 and onboard via-sata.
>0000:00:00.0 Host bridge: VIA Technologies, Inc. VT8377 [KT400/KT600 AGP] Host
>Bridge (rev 80)
>0000:00:01.0 PCI bridge: VIA Technologies, Inc. VT8237 PCI Bridge
>0000:00:0d.0 Unknown mass storage controller: Promise Technology, Inc. PDC20318
>(SATA150 TX4) (rev 02)
>0000:00:0f.1 IDE interface: VIA Technologies, Inc.
>VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06)
>
>- which was the last kernel version prior to 2.6.22-rc4 that worked
>2.6.22-rc3
>- the kernel messages up to the hang, if you can capture them
>Easier once I learned patience...
>
>sata_promise 0000:00:0d.0: version 2.07
>ACPI: PCI Interrupt 0000:00:0d.0[A] -> GSI 16 (level, low) -> IRQ 17
>scsi0 : sata_promise
>scsi1 : sata_promise
>scsi2 : sata_promise
>scsi3 : sata_promise
>ata1: SATA max UDMA/133 cmd 0xf880a200 ctl 0xf880a238 bmdma 0x00000000 irq 0
>ata2: SATA max UDMA/133 cmd 0xf880a280 ctl 0xf880a2b8 bmdma 0x00000000 irq 0
>ata3: SATA max UDMA/133 cmd 0xf880a300 ctl 0xf880a338 bmdma 0x00000000 irq 0
>ata4: SATA max UDMA/133 cmd 0xf880a380 ctl 0xf880a3b8 bmdma 0x00000000 irq 0
>ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
>ata1.00: ata_hpa_resize 1: sectors = 490234752, hpa_sectors = 490234752
>ata1.00: ATA-7: Maxtor 6B250S0, BANC19J0, max UDMA/133
>ata1.00: 490234752 sectors, multi 0: LBA48 NCQ (depth 0/32)
>ata1.00: qc timeout (cmd 0xef)
>ata1.00: failed to set xfermode (err_mask=0x4)
http://bugzilla.kernel.org/show_bug.cgi?id=8587 has a similar
report: it lists 2.6.22-rc3-git7 as OK but 2.6.22-rc4 as broken.
I can easily reproduce the problem in 2.6.22-rc4. There are no
sata_promise changes between rc3 and rc4, but Tejun's libata
polling SETXFER change was included in rc4. Reverting it makes
sata_promise work again for me.
I suspect that sata_promise.c:pdc_interrupt() should detect
a qc w/ ATA_TFLAG_POLLING, treat the interrupt as spurious,
and just call ata_chk_status(qc), similar to how sata_inic162x.c,
sata_nv.c, sata_sil.c, and sata_vsc.c do things.
Jeff/Tejun: comments?
/Mikael
On Tue, Jun 05, 2007 at 11:31:46PM +0200, Mikael Pettersson wrote:
> I can easily reproduce the problem in 2.6.22-rc4. There are no
> sata_promise changes between rc3 and rc4, but Tejun's libata
> polling SETXFER change was included in rc4. Reverting it makes
> sata_promise work again for me.
Ugh.
> I suspect that sata_promise.c:pdc_interrupt() should detect
> a qc w/ ATA_TFLAG_POLLING, treat the interrupt as spurious,
> and just call ata_chk_status(qc), similar to how sata_inic162x.c,
> sata_nv.c, sata_sil.c, and sata_vsc.c do things.
Yes, highly likely.
SFF-like controllers (and in this case, Promise is included in that
list) with their own interrupt handlers need their own polling handling
code.
Jeff
Jeff Garzik wrote:
> On Tue, Jun 05, 2007 at 11:31:46PM +0200, Mikael Pettersson wrote:
>> I can easily reproduce the problem in 2.6.22-rc4. There are no
>> sata_promise changes between rc3 and rc4, but Tejun's libata
>> polling SETXFER change was included in rc4. Reverting it makes
>> sata_promise work again for me.
>
> Ugh.
Reverting Tejun's patch also fixed my boot failure.
00:08.0 RAID bus controller: Promise Technology, Inc. PDC20376 (FastTrak
376) (rev 02)
Subsystem: ASUSTeK Computer Inc. A7V8X motherboard
Flags: bus master, 66MHz, medium devsel, latency 96, IRQ 16
I/O ports at d400 [size=64]
I/O ports at d000 [size=16]
I/O ports at b800 [size=128]
Memory at f6800000 (32-bit, non-prefetchable) [size=4K]
Memory at f6000000 (32-bit, non-prefetchable) [size=128K]
Capabilities: [60] Power Management version 2
00:11.1 IDE interface: VIA Technologies, Inc.
VT82C586A/B/VT82C686/A/B/VT823x/A/C PIPC Bus Master IDE (rev 06) (prog-if
8a [Master SecP PriP])
Subsystem: ASUSTeK Computer Inc. A7V8X / A7V333 motherboard
Flags: bus master, medium devsel, latency 32, IRQ 255
[virtual] Memory at 000001f0 (32-bit, non-prefetchable) [size=8]
[virtual] Memory at 000003f0 (type 3, non-prefetchable) [size=1]
[virtual] Memory at 00000170 (32-bit, non-prefetchable) [size=8]
[virtual] Memory at 00000370 (type 3, non-prefetchable) [size=1]
I/O ports at a400 [size=16]
Capabilities: [c0] Power Management version 2
The problem is that the controller can't be initialized properly, so
the kernel keeps trying every 5 seconds. I gave up after about five
failures. If the actual boot error messages would help I'll need to
copy them by hand -- just say the word.
Hello,
Jeff Garzik wrote:
> On Tue, Jun 05, 2007 at 11:31:46PM +0200, Mikael Pettersson wrote:
>> I can easily reproduce the problem in 2.6.22-rc4. There are no
>> sata_promise changes between rc3 and rc4, but Tejun's libata
>> polling SETXFER change was included in rc4. Reverting it makes
>> sata_promise work again for me.
>
> Ugh.
Ugh...
>> I suspect that sata_promise.c:pdc_interrupt() should detect
>> a qc w/ ATA_TFLAG_POLLING, treat the interrupt as spurious,
>> and just call ata_chk_status(qc), similar to how sata_inic162x.c,
>> sata_nv.c, sata_sil.c, and sata_vsc.c do things.
>
> Yes, highly likely.
I'm not sure whether that will work. I'll give a shot at it here.
> SFF-like controllers (and in this case, Promise is included in that
> list) with their own interrupt handlers need their own polling handling
> code.
One thing I don't understand is why IDENTIFY works. IDENTIFY uses
polling too. Hmm...
--
tejun
sata_promise uses two different command modes - packet and TF. Packet
mode is intelligent low-overhead mode while TF is the same old
taskfile interface. As with other advanced interface (ahci/sil24),
ATA_TFLAG_POLLING has no effect in packet mode. However, PIO commands
are issued using TF interface in polling mode, so pdc_interrupt()
considers interrupts spurious if ATA_TFLAG_POLLING is set.
This is broken for polling NODATA commands because command is issued
using packet mode but the interrupt handler ignores it due to
ATA_TFLAG_POLLING. Fix pdc_qc_issue_prot() such that ATA/ATAPI NODATA
commands are issued using TF interface if ATA_TFLAG_POLLING is set.
This patch fixes detection failure introduced by polling SETXFERMODE.
Signed-off-by: Tejun Heo <[email protected]>
---
David, please verify this patch. Mikael, does this look okay? Please
push this upstream after David and Mikael's ack.
(This repost is identical to the previous posting but it's now on the
correct thread.)
Thanks.
drivers/ata/sata_promise.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
index 2b924a6..6dc0b01 100644
--- a/drivers/ata/sata_promise.c
+++ b/drivers/ata/sata_promise.c
@@ -784,9 +784,12 @@ static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
if (qc->dev->flags & ATA_DFLAG_CDB_INTR)
break;
/*FALLTHROUGH*/
+ case ATA_PROT_NODATA:
+ if (qc->tf.flags & ATA_TFLAG_POLLING)
+ break;
+ /*FALLTHROUGH*/
case ATA_PROT_ATAPI_DMA:
case ATA_PROT_DMA:
- case ATA_PROT_NODATA:
pdc_packet_start(qc);
return 0;
@@ -800,7 +803,7 @@ static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
{
WARN_ON (tf->protocol == ATA_PROT_DMA ||
- tf->protocol == ATA_PROT_NODATA);
+ tf->protocol == ATA_PROT_ATAPI_DMA);
ata_tf_load(ap, tf);
}
@@ -808,7 +811,7 @@ static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
{
WARN_ON (tf->protocol == ATA_PROT_DMA ||
- tf->protocol == ATA_PROT_NODATA);
+ tf->protocol == ATA_PROT_ATAPI_DMA);
ata_exec_command(ap, tf);
}
On Wed, 6 Jun 2007 19:21:22 +0900, Tejun Heo wrote:
> sata_promise uses two different command modes - packet and TF. Packet
> mode is intelligent low-overhead mode while TF is the same old
> taskfile interface. As with other advanced interface (ahci/sil24),
> ATA_TFLAG_POLLING has no effect in packet mode. However, PIO commands
> are issued using TF interface in polling mode, so pdc_interrupt()
> considers interrupts spurious if ATA_TFLAG_POLLING is set.
>
> This is broken for polling NODATA commands because command is issued
> using packet mode but the interrupt handler ignores it due to
> ATA_TFLAG_POLLING. Fix pdc_qc_issue_prot() such that ATA/ATAPI NODATA
> commands are issued using TF interface if ATA_TFLAG_POLLING is set.
>
> This patch fixes detection failure introduced by polling SETXFERMODE.
>
> Signed-off-by: Tejun Heo <[email protected]>
> ---
> David, please verify this patch. Mikael, does this look okay? Please
> push this upstream after David and Mikael's ack.
Thanks Tejun.
I totally agree with the analysis and the solution. I considered bouncing
polled commands back to libata, but Tejun beat me to it :-)
Tested successfully with SATA and PATA disks on 20619, 20575, 20775,
and 40718 chips.
Acked-by: Mikael Pettersson <[email protected]>
/Mikael
>
> (This repost is identical to the previous posting but it's now on the
> correct thread.)
>
> Thanks.
>
> drivers/ata/sata_promise.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/ata/sata_promise.c b/drivers/ata/sata_promise.c
> index 2b924a6..6dc0b01 100644
> --- a/drivers/ata/sata_promise.c
> +++ b/drivers/ata/sata_promise.c
> @@ -784,9 +784,12 @@ static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
> if (qc->dev->flags & ATA_DFLAG_CDB_INTR)
> break;
> /*FALLTHROUGH*/
> + case ATA_PROT_NODATA:
> + if (qc->tf.flags & ATA_TFLAG_POLLING)
> + break;
> + /*FALLTHROUGH*/
> case ATA_PROT_ATAPI_DMA:
> case ATA_PROT_DMA:
> - case ATA_PROT_NODATA:
> pdc_packet_start(qc);
> return 0;
>
> @@ -800,7 +803,7 @@ static unsigned int pdc_qc_issue_prot(struct ata_queued_cmd *qc)
> static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
> {
> WARN_ON (tf->protocol == ATA_PROT_DMA ||
> - tf->protocol == ATA_PROT_NODATA);
> + tf->protocol == ATA_PROT_ATAPI_DMA);
> ata_tf_load(ap, tf);
> }
>
> @@ -808,7 +811,7 @@ static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
> static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
> {
> WARN_ON (tf->protocol == ATA_PROT_DMA ||
> - tf->protocol == ATA_PROT_NODATA);
> + tf->protocol == ATA_PROT_ATAPI_DMA);
> ata_exec_command(ap, tf);
> }
>
>
Tejun Heo wrote:
> ... Fix pdc_qc_issue_prot() such that ATA/ATAPI NODATA
> commands are issued using TF interface if ATA_TFLAG_POLLING is set.
>
> This patch fixes detection failure introduced by polling SETXFERMODE.
Your patch works for me, thanks.
FYI to all --
As a reminder. the Promise hardware programs registers when it receives
a SET FEATURES - XFER MODE.
If data transfer is occurring on OTHER ports at the time this is issued,
then data corruption is guaranteed to occur. Polling will not fix this
problem -- all ports need to be inactive, when a SET FEATURES - XFER
MODE command is issued for any port.
Jeff
On Wed, 6 Jun 2007 12:05:11 -0400
Jeff Garzik <[email protected]> wrote:
>
> FYI to all --
>
> As a reminder. the Promise hardware programs registers when it receives
> a SET FEATURES - XFER MODE.
>
> If data transfer is occurring on OTHER ports at the time this is issued,
> then data corruption is guaranteed to occur. Polling will not fix this
> problem -- all ports need to be inactive, when a SET FEATURES - XFER
> MODE command is issued for any port.
Cool - will whoever makes that work drop me a note as I need to do the
same eventually for a couple of other controllers.
On Wed, Jun 06, 2007 at 07:21:22PM +0900, Tejun Heo wrote:
> sata_promise uses two different command modes - packet and TF. Packet
> mode is intelligent low-overhead mode while TF is the same old
> taskfile interface. As with other advanced interface (ahci/sil24),
> ATA_TFLAG_POLLING has no effect in packet mode. However, PIO commands
> are issued using TF interface in polling mode, so pdc_interrupt()
> considers interrupts spurious if ATA_TFLAG_POLLING is set.
>
> This is broken for polling NODATA commands because command is issued
> using packet mode but the interrupt handler ignores it due to
> ATA_TFLAG_POLLING. Fix pdc_qc_issue_prot() such that ATA/ATAPI NODATA
> commands are issued using TF interface if ATA_TFLAG_POLLING is set.
>
> This patch fixes detection failure introduced by polling SETXFERMODE.
>
> Signed-off-by: Tejun Heo <[email protected]>
> ---
> David, please verify this patch. Mikael, does this look okay? Please
> push this upstream after David and Mikael's ack.
>
> (This repost is identical to the previous posting but it's now on the
> correct thread.)
Acked-by: Jeff Garzik <[email protected]>
Tejun, would you mind pushing this upstream to Linus/Andrew?
I'm travelling this week, and my home firewall doesn't like me.
It is probably easier for you than me.
Jeff
On Thu, 7 Jun 2007 11:17:17 -0400 Jeff Garzik <[email protected]> wrote:
> On Wed, Jun 06, 2007 at 07:21:22PM +0900, Tejun Heo wrote:
> > sata_promise uses two different command modes - packet and TF. Packet
> > mode is intelligent low-overhead mode while TF is the same old
> > taskfile interface. As with other advanced interface (ahci/sil24),
> > ATA_TFLAG_POLLING has no effect in packet mode. However, PIO commands
> > are issued using TF interface in polling mode, so pdc_interrupt()
> > considers interrupts spurious if ATA_TFLAG_POLLING is set.
> >
> > This is broken for polling NODATA commands because command is issued
> > using packet mode but the interrupt handler ignores it due to
> > ATA_TFLAG_POLLING. Fix pdc_qc_issue_prot() such that ATA/ATAPI NODATA
> > commands are issued using TF interface if ATA_TFLAG_POLLING is set.
> >
> > This patch fixes detection failure introduced by polling SETXFERMODE.
> >
> > Signed-off-by: Tejun Heo <[email protected]>
> > ---
> > David, please verify this patch. Mikael, does this look okay? Please
> > push this upstream after David and Mikael's ack.
> >
> > (This repost is identical to the previous posting but it's now on the
> > correct thread.)
>
> Acked-by: Jeff Garzik <[email protected]>
>
> Tejun, would you mind pushing this upstream to Linus/Andrew?
>
> I'm travelling this week, and my home firewall doesn't like me.
> It is probably easier for you than me.
>
I added the below to my next batch:
From: Tejun Heo <[email protected]>
sata_promise uses two different command modes - packet and TF. Packet mode
is intelligent low-overhead mode while TF is the same old taskfile
interface. As with other advanced interface (ahci/sil24),
ATA_TFLAG_POLLING has no effect in packet mode. However, PIO commands are
issued using TF interface in polling mode, so pdc_interrupt() considers
interrupts spurious if ATA_TFLAG_POLLING is set.
This is broken for polling NODATA commands because command is issued using
packet mode but the interrupt handler ignores it due to ATA_TFLAG_POLLING.
Fix pdc_qc_issue_prot() such that ATA/ATAPI NODATA commands are issued
using TF interface if ATA_TFLAG_POLLING is set.
This patch fixes detection failure introduced by polling SETXFERMODE.
Signed-off-by: Tejun Heo <[email protected]>
Acked-by: Mikael Pettersson <[email protected]>
Acked-by: Jeff Garzik <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
drivers/ata/sata_promise.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff -puN drivers/ata/sata_promise.c~sata_promise-use-tf-interface-for-polling-nodata-commands drivers/ata/sata_promise.c
--- a/drivers/ata/sata_promise.c~sata_promise-use-tf-interface-for-polling-nodata-commands
+++ a/drivers/ata/sata_promise.c
@@ -784,9 +784,12 @@ static unsigned int pdc_qc_issue_prot(st
if (qc->dev->flags & ATA_DFLAG_CDB_INTR)
break;
/*FALLTHROUGH*/
+ case ATA_PROT_NODATA:
+ if (qc->tf.flags & ATA_TFLAG_POLLING)
+ break;
+ /*FALLTHROUGH*/
case ATA_PROT_ATAPI_DMA:
case ATA_PROT_DMA:
- case ATA_PROT_NODATA:
pdc_packet_start(qc);
return 0;
@@ -800,7 +803,7 @@ static unsigned int pdc_qc_issue_prot(st
static void pdc_tf_load_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
{
WARN_ON (tf->protocol == ATA_PROT_DMA ||
- tf->protocol == ATA_PROT_NODATA);
+ tf->protocol == ATA_PROT_ATAPI_DMA);
ata_tf_load(ap, tf);
}
@@ -808,7 +811,7 @@ static void pdc_tf_load_mmio(struct ata_
static void pdc_exec_command_mmio(struct ata_port *ap, const struct ata_taskfile *tf)
{
WARN_ON (tf->protocol == ATA_PROT_DMA ||
- tf->protocol == ATA_PROT_NODATA);
+ tf->protocol == ATA_PROT_ATAPI_DMA);
ata_exec_command(ap, tf);
}
_
On 06/06/2007 12:05 PM, Jeff Garzik wrote:
> FYI to all --
>
> As a reminder. the Promise hardware programs registers when it receives
> a SET FEATURES - XFER MODE.
>
> If data transfer is occurring on OTHER ports at the time this is issued,
> then data corruption is guaranteed to occur. Polling will not fix this
> problem -- all ports need to be inactive, when a SET FEATURES - XFER
> MODE command is issued for any port.
>
So is this patch OK but yet more work needs to be done, or does
this patch cause new problems?
On Thu, Jun 07, 2007 at 01:20:13PM -0400, Chuck Ebbert wrote:
> On 06/06/2007 12:05 PM, Jeff Garzik wrote:
> > FYI to all --
> >
> > As a reminder. the Promise hardware programs registers when it receives
> > a SET FEATURES - XFER MODE.
> >
> > If data transfer is occurring on OTHER ports at the time this is issued,
> > then data corruption is guaranteed to occur. Polling will not fix this
> > problem -- all ports need to be inactive, when a SET FEATURES - XFER
> > MODE command is issued for any port.
> So is this patch OK but yet more work needs to be done, or does
> this patch cause new problems?
Causes no /new/ problems... :) The existing problem described above
remains.
Jeff
On Thu, Jun 07, 2007 at 01:25:07PM -0400, Jeff Garzik wrote:
> On Thu, Jun 07, 2007 at 01:20:13PM -0400, Chuck Ebbert wrote:
> > On 06/06/2007 12:05 PM, Jeff Garzik wrote:
> > > FYI to all --
> > >
> > > As a reminder. the Promise hardware programs registers when it receives
> > > a SET FEATURES - XFER MODE.
> > >
> > > If data transfer is occurring on OTHER ports at the time this is issued,
> > > then data corruption is guaranteed to occur. Polling will not fix this
> > > problem -- all ports need to be inactive, when a SET FEATURES - XFER
> > > MODE command is issued for any port.
>
> > So is this patch OK but yet more work needs to be done, or does
> > this patch cause new problems?
>
> Causes no /new/ problems... :) The existing problem described above
> remains.
Just to expand... this problem doesn't really affect a lot of users in
the majority case, since we do speed tuning before data transfer starts.
The main problem that remains is the rare (but no less important) case
where libata-EH will tune speed during operation in respond to certain
classes of errors.
Jeff