public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* Re: Linux 2.6.22-rc4 - sata_promise regression since -rc3
@ 2007-06-05 21:31 Mikael Pettersson
  2007-06-05 21:52 ` Jeff Garzik
  0 siblings, 1 reply; 14+ messages in thread
From: Mikael Pettersson @ 2007-06-05 21:31 UTC (permalink / raw)
  To: akpm, david, htejun, jean.luc.coulon, jgarzik,
	michal.k.k.piotrowski, torvalds
  Cc: linux-kernel, mikpe

On Tue, 05 Jun 2007 17:14:46 +0100, David Greaves <david@dgreaves.com> 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

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [REPOST PATCH] sata_promise: use TF interface for polling NODATA commands
@ 2007-06-06 11:11 Mikael Pettersson
  0 siblings, 0 replies; 14+ messages in thread
From: Mikael Pettersson @ 2007-06-06 11:11 UTC (permalink / raw)
  To: htejun, jeff
  Cc: akpm, david, jean.luc.coulon, jgarzik, linux-kernel,
	michal.k.k.piotrowski, mikpe, torvalds

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 <htejun@gmail.com>
> ---
> 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 <mikpe@it.uu.se>

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2007-06-07 17:31 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-06-05 21:31 Linux 2.6.22-rc4 - sata_promise regression since -rc3 Mikael Pettersson
2007-06-05 21:52 ` Jeff Garzik
2007-06-05 23:37   ` walt
2007-06-06  6:56   ` Tejun Heo
2007-06-06 10:21     ` [REPOST PATCH] sata_promise: use TF interface for polling NODATA commands Tejun Heo
2007-06-06 15:42       ` walt
2007-06-06 16:05       ` Jeff Garzik
2007-06-06 16:34         ` Alan Cox
2007-06-07 17:20         ` Chuck Ebbert
2007-06-07 17:25           ` Jeff Garzik
2007-06-07 17:30             ` Jeff Garzik
2007-06-07 15:17       ` Jeff Garzik
2007-06-07 15:45         ` Andrew Morton
  -- strict thread matches above, loose matches on Subject: below --
2007-06-06 11:11 Mikael Pettersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox