NetBSD Problem Report #47389

From o.vd.linden@quicknet.nl  Tue Jan  1 09:41:17 2013
Return-Path: <o.vd.linden@quicknet.nl>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id C4D2A63E648
	for <gnats-bugs@gnats.NetBSD.org>; Tue,  1 Jan 2013 09:41:16 +0000 (UTC)
Message-Id: <20130101094109.GA1972@sheep>
Date: Tue, 1 Jan 2013 10:41:09 +0100
From: Onno van der Linden <o.vd.linden@quicknet.nl>
To: gnats-bugs@gnats.NetBSD.org
Subject: lost interrupt messages with VT6421A and sata atapi device

>Number:         47389
>Category:       kern
>Synopsis:       lost interrupt messages with viaide(4) and sata atapi device
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Jan 01 09:45:01 +0000 2013
>Last-Modified:  Mon Jan 07 16:55:04 +0000 2013
>Originator:     Onno van der Linden
>Release:        NetBSD 6.99.16
>Organization:

>Environment:


System: NetBSD sheep 6.99.16 NetBSD 6.99.16 (SHEEP) #38: Tue Jan 1 09:50:29 MET 2013 root@sheep:/usr/src/sys/arch/i386/compile/SHEEP i386
Architecture: i386
Machine: i386
>Description:
With cd0 an atapi drive attached to the sata port of a via VT6421A
scsictl cd0 identify
results in
viaide0:1:0: lost interrupt
	type: atapi tc_bcount: 0 tc_skip: 74
>How-To-Repeat:
scsictl cd0 identify
>Fix:
Somehow the VT6421A doesn't like doing a PIO transfer with
both 32-bit and 16-bit I/O intructions as done in wdc_datain_pio
en wdc_dataout_pio. Fix it by checking the alignment
of the len parameter and do 32/16/8 - bit pio instructions all the way.

*** /sys/dev/pci/viaide.c.orig	Mon Dec 31 20:38:58 2012
--- /sys/dev/pci/viaide.c	Tue Jan  1 09:47:42 2013
***************
*** 58,63 ****
--- 58,68 ----
  		    const struct pci_attach_args *);
  static void	via_setup_channel(struct ata_channel *);

+ static void	via_datain_pio(struct ata_channel *chp,
+ 			int flags, void *bf, size_t len);
+ static void	via_dataout_pio(struct ata_channel *chp,
+ 			int flags, void *bf, size_t len);
+ 
  static int	viaide_match(device_t, cfdata_t, void *);
  static void	viaide_attach(device_t, device_t, void *);
  static const struct pciide_product_desc *
***************
*** 1118,1123 ****
--- 1123,1130 ----
  		sc->sc_wdcdev.sc_atac.atac_dma_cap = 2;
  		sc->sc_wdcdev.sc_atac.atac_udma_cap = 6;
  	}
+ 	sc->sc_wdcdev.datain_pio = via_datain_pio;
+ 	sc->sc_wdcdev.dataout_pio = via_dataout_pio;
  	sc->sc_wdcdev.sc_atac.atac_set_modes = sata_setup_channel;

  	sc->sc_wdcdev.sc_atac.atac_channels = sc->wdc_chanarray;
***************
*** 1211,1213 ****
--- 1218,1398 ----
  		wdcattach(wdc_cp);
  	}
  }
+ 
+ static void
+ via_datain_pio(struct ata_channel *chp, int flags, void *bf, size_t len)
+ {
+ 	struct wdc_regs *wdr = CHAN_TO_WDC_REGS(chp);
+ 
+ #ifndef __NO_STRICT_ALIGNMENT
+ 	if ((uintptr_t)bf & 1)
+ 		goto unaligned;
+ 	if ((flags & ATA_DRIVE_CAP32) && ((uintptr_t)bf & 3))
+ 		goto unaligned;
+ #endif
+ 
+ 	if (flags & ATA_DRIVE_NOSTREAM) {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			bus_space_read_multi_4(wdr->data32iot,
+ 			    wdr->data32ioh, 0, bf, len >> 2);
+ 		}
+ 		else if ((len & 1) == 0) {
+ 			bus_space_read_multi_2(wdr->cmd_iot,
+ 			    wdr->cmd_iohs[wd_data], 0, bf, len >> 1);
+ 		}
+ 		else  {
+ 			bus_space_read_multi_1(wdr->cmd_iot,
+ 			    wdr->cmd_iohs[wd_data], 0, bf, len);
+ 		}
+ 	} else {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			bus_space_read_multi_stream_4(wdr->data32iot,
+ 			    wdr->data32ioh, 0, bf, len >> 2);
+ 		}
+ 		else if ((len & 1) == 0) {
+ 			bus_space_read_multi_stream_2(wdr->cmd_iot,
+ 			    wdr->cmd_iohs[wd_data], 0, bf, len >> 1);
+ 		}
+ 		else  {
+ 			bus_space_read_multi_stream_1(wdr->cmd_iot,
+ 			    wdr->cmd_iohs[wd_data], 0, bf, len);
+ 		}
+ 	}
+ 	return;
+ 
+ #ifndef __NO_STRICT_ALIGNMENT
+ unaligned:
+ 	if (flags & ATA_DRIVE_NOSTREAM) {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			while (len > 3) {
+ 				uint32_t val;
+ 
+ 				val = bus_space_read_4(wdr->data32iot,
+ 				    wdr->data32ioh, 0);
+ 				memcpy(bf, &val, 4);
+ 				bf = (char *)bf + 4;
+ 				len -= 4;
+ 			}
+ 		}
+ 		else {
+ 			while (len > 1) {
+ 				uint16_t val;
+ 
+ 				val = bus_space_read_2(wdr->cmd_iot,
+ 				    wdr->cmd_iohs[wd_data], 0);
+ 				memcpy(bf, &val, 2);
+ 				bf = (char *)bf + 2;
+ 				len -= 2;
+ 			}
+ 		}
+ 	} else {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			while (len > 3) {
+ 				uint32_t val;
+ 
+ 				val = bus_space_read_stream_4(wdr->data32iot,
+ 				    wdr->data32ioh, 0);
+ 				memcpy(bf, &val, 4);
+ 				bf = (char *)bf + 4;
+ 				len -= 4;
+ 			}
+ 		}
+ 		else {
+ 			while (len > 1) {
+ 				uint16_t val;
+ 
+ 				val = bus_space_read_stream_2(wdr->cmd_iot,
+ 				    wdr->cmd_iohs[wd_data], 0);
+ 				memcpy(bf, &val, 2);
+ 				bf = (char *)bf + 2;
+ 				len -= 2;
+ 			}
+ 		}
+ 	}
+ #endif
+ }
+ 
+ static void
+ via_dataout_pio(struct ata_channel *chp, int flags, void *bf, size_t len)
+ {
+ 	struct wdc_regs *wdr = CHAN_TO_WDC_REGS(chp);
+ 
+ #ifndef __NO_STRICT_ALIGNMENT
+ 	if ((uintptr_t)bf & 1)
+ 		goto unaligned;
+ 	if ((flags & ATA_DRIVE_CAP32) && ((uintptr_t)bf & 3))
+ 		goto unaligned;
+ #endif
+ 
+ 	if (flags & ATA_DRIVE_NOSTREAM) {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			bus_space_write_multi_4(wdr->data32iot,
+ 			    wdr->data32ioh, 0, bf, len >> 2);
+ 		}
+ 		else {
+ 			bus_space_write_multi_2(wdr->cmd_iot,
+ 			    wdr->cmd_iohs[wd_data], 0, bf, len >> 1);
+ 		}
+ 	} else {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			bus_space_write_multi_stream_4(wdr->data32iot,
+ 			    wdr->data32ioh, 0, bf, len >> 2);
+ 		}
+ 		else {
+ 			bus_space_write_multi_stream_2(wdr->cmd_iot,
+ 			    wdr->cmd_iohs[wd_data], 0, bf, len >> 1);
+ 		}
+ 	}
+ 	return;
+ 
+ #ifndef __NO_STRICT_ALIGNMENT
+ unaligned:
+ 	if (flags & ATA_DRIVE_NOSTREAM) {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			while (len > 3) {
+ 				uint32_t val;
+ 
+ 				memcpy(&val, bf, 4);
+ 				bus_space_write_4(wdr->data32iot,
+ 				    wdr->data32ioh, 0, val);
+ 				bf = (char *)bf + 4;
+ 				len -= 4;
+ 			}
+ 		}
+ 		else {
+ 			while (len > 1) {
+ 				uint16_t val;
+ 
+ 				memcpy(&val, bf, 2);
+ 				bus_space_write_2(wdr->cmd_iot,
+ 				    wdr->cmd_iohs[wd_data], 0, val);
+ 				bf = (char *)bf + 2;
+ 				len -= 2;
+ 			}
+ 		}
+ 	} else {
+ 		if ((flags & ATA_DRIVE_CAP32) && (len & 3) == 0) {
+ 			while (len > 3) {
+ 				uint32_t val;
+ 
+ 				memcpy(&val, bf, 4);
+ 				bus_space_write_stream_4(wdr->data32iot,
+ 				    wdr->data32ioh, 0, val);
+ 				bf = (char *)bf + 4;
+ 				len -= 4;
+ 			}
+ 		}
+ 		else {
+ 			while (len > 1) {
+ 				uint16_t val;
+ 
+ 				memcpy(&val, bf, 2);
+ 				bus_space_write_stream_2(wdr->cmd_iot,
+ 				    wdr->cmd_iohs[wd_data], 0, val);
+ 				bf = (char *)bf + 2;
+ 				len -= 2;
+ 			}
+ 		}
+ 	}
+ #endif 
+ }

>Audit-Trail:
From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/47389: lost interrupt messages with VT6421A and sata atapi device
Date: Tue, 1 Jan 2013 15:58:35 +0000

 On Tue, Jan 01, 2013 at 09:45:01AM +0000, Onno van der Linden wrote:
 > >Number:         47389
 > >Category:       kern
 > >Synopsis:       lost interrupt messages with viaide(4) and sata atapi device
 ...
 > Somehow the VT6421A doesn't like doing a PIO transfer with
 > both 32-bit and 16-bit I/O intructions as done in wdc_datain_pio
 > en wdc_dataout_pio. Fix it by checking the alignment
 > of the len parameter and do 32/16/8 - bit pio instructions all the way.

 Might be worth using a new ATA_DRIVE_xxx flag and slightly
 modifying the tests in wdc_datain/out_pio() rather than
 replicating the code.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: Onno van der Linden <o.vd.linden@quicknet.nl>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/47389: lost interrupt messages with VT6421A and sata atapi device
Date: Tue, 1 Jan 2013 17:04:21 +0100

 On Tue, Jan 01, 2013 at 03:45:03PM +0000, David Laight wrote:
 >  Might be worth using a new ATA_DRIVE_xxx flag and slightly
 >  modifying the tests in wdc_datain/out_pio() rather than
 >  replicating the code.

 Or not use an extra flag at all and change wdc_datain/out_pio()
 to look like the via_data{in,out}_pio versions. Most of the time
 len is a multiple of 4 anyway and if it isn't, it should be so
 small (I've seen 34 bytes with a mount_cd9660 and 74 bytes
 with the scsictl identify)  to not make much of a performance
 difference because 32-bit transfers aren't used in those case.

 Onno

From: Manuel Bouyer <bouyer@antioche.eu.org>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org,
        Onno van der Linden <o.vd.linden@quicknet.nl>
Subject: Re: kern/47389: lost interrupt messages with VT6421A and sata atapi
 device
Date: Mon, 7 Jan 2013 17:53:17 +0100

 On Tue, Jan 01, 2013 at 04:05:03PM +0000, Onno van der Linden wrote:
 >  [...]
 >  Or not use an extra flag at all and change wdc_datain/out_pio()
 >  to look like the via_data{in,out}_pio versions. Most of the time
 >  len is a multiple of 4 anyway and if it isn't, it should be so
 >  small (I've seen 34 bytes with a mount_cd9660 and 74 bytes
 >  with the scsictl identify)  to not make much of a performance
 >  difference because 32-bit transfers aren't used in those case.

 yes, that's probably the best way.

 -- 
 Manuel Bouyer <bouyer@antioche.eu.org>
      NetBSD: 26 ans d'experience feront toujours la difference
 --

>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.