NetBSD Problem Report #56403

From www@netbsd.org  Wed Sep 15 15:31:08 2021
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 0A8651A921F
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 15 Sep 2021 15:31:08 +0000 (UTC)
Message-Id: <20210915153106.6926B1A923A@mollari.NetBSD.org>
Date: Wed, 15 Sep 2021 15:31:06 +0000 (UTC)
From: rokuyama.rk@gmail.com
Reply-To: rokuyama.rk@gmail.com
To: gnats-bugs@NetBSD.org
Subject: memory mapped mode of wdc(4) at pcmcia(4) broken after jdolecek-ncq branch merge
X-Send-Pr-Version: www-1.0

>Number:         56403
>Category:       kern
>Synopsis:       memory mapped mode of wdc(4) at pcmcia(4) broken after jdolecek-ncq branch merge
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Sep 15 15:35:00 +0000 2021
>Closed-Date:    Fri Jul 21 08:51:33 +0000 2023
>Last-Modified:  Fri Jul 21 08:51:33 +0000 2023
>Originator:     Rin Okuyama
>Release:        9.99.88
>Organization:
Department of Physics, Meiji University
>Environment:
NetBSD j690 9.99.88 NetBSD 9.99.88 (J690NE) #10: Wed Sep 15 21:23:22 JST 2021  rin@latipes:/sys/arch/hpcsh/compile/J690NE hpcsh
>Description:
For -current and netbsd-9, compact flash card in CF slot of Jornada 690
(hpcsh) does not work, whereas the same card works just fine if inserted
to PCMCIA slot.

This card works both for CF and PCMCIA slots for netbsd-8. Diff between
dmesg for two slots is:

CF slot:
https://gist.github.com/rokuyama/a790ef280e61a98f8b692cd51051f6c8

PCMCIA slot:
https://gist.github.com/rokuyama/e18671a331c91174dae634355f819845

----
-wdc0 at pcmcia1 function 0: <Kingston, CF_Card, >
-wdc0: memory mapped mode
-atabus0 at wdc0 channel 0
-wd0 at atabus0 drive 0
+wdc1 at pcmcia0 function 0: <Kingston, CF_Card, >
+wdc1: i/o mapped mode
+atabus1 at wdc1 channel 0
+wd0 at atabus1 drive 0
----

This is because CF slot (pcmcia1) supports only memory mapped mode,
while PCMCIA slot (pcmcia0) supports both memory and I/O mapped modes.

``atactl wd0 identify'' does not change for both slots:

----
Model: CF_Card, Rev: Ver7.02K, Serial #:     5B5911CD0
Device type: CF-ATA
Capacity 7994 Mbytes, 15613920 sectors, 512 bytes/sector
Cylinders: 15490, heads: 16, sec/track: 63
Device capabilities:
	LBA
	ATA standby timer values
	IORDY operation
Device supports following standards:
ATA-4 ATA-5 
Command set support:
	NOP command (enabled)
	READ BUFFER command (enabled)
	WRITE BUFFER command (enabled)
	Write cache (enabled)
	Power Management feature set (disabled)
	Security Mode feature set (disabled)
	SMART feature set (disabled)
	FLUSH CACHE command (enabled)
	48-bit Address feature set (enabled)
	CFA feature set (enabled)
----

I have narrowed down that this failure starts when jdolecek-ncq branch
was merged (8.99.4).

Here is some unorganized console capture for -current with ATADEBUG, when
CF slot is used; NFS root works, disklabel(8) to that card also works, but
mount(8) freezes, and cannot reboot/halt the system.

https://gist.github.com/rokuyama/e0d120dc62dad2ece8574f83de279be9
>How-To-Repeat:
Try to mount CF cards in CF slot of Jornada 690 or 680.
>Fix:
N/A

I've confirmed that there is no alignment faults for
wdc_pcmcia_data{in,out}_memory(). Also, we should enable ATAC_CAP_DATA32
flag also for memory-mapped mode, but this does not improve the situation.

>Release-Note:

>Audit-Trail:
From: Rin Okuyama <rokuyama.rk@gmail.com>
To: "gnats-bugs@NetBSD.org" <gnats-bugs@NetBSD.org>
Cc: jdolecek@netbsd.org
Subject: Re: kern/56403 (memory mapped mode of wdc(4) at pcmcia(4) broken
 after jdolecek-ncq branch merge)
Date: Mon, 4 Oct 2021 15:57:01 +0900

 I've found it!

 Before jdolecek-ncq merge, for ATA_POLL, _wdc_ata_bio_start() iterated
 itself and wdc_ata_bio_intr(), until ATA_ITSDONE is achieved.

 This behavior has been lost accidentally when ata_xfer_ops:c_poll was
 introduced:

 http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ata/ata_wdc.c#rev1.106

 In order to fix this, I've changed ata_xfer_ops:c_poll from void to
 int, and made wdc_ata_bio_poll() return whether it is to be called
 again or not:

 https://gist.github.com/rokuyama/72e261f74d8311cfbef9c701f60d5260

 With this patch, wdc(4) at pcmcia(4) (memory-mapped mode) becomes
 working again. And other wdc(4) variants with ATAC_CAP_NOIRQ should
 be fixed too.

 Alternatively, we can check ATA_ITSDONE flag in ata_xfer_start()
 with ata_xfer_ops:c_poll being unchanged. But, it is clearer to
 check that flag in c_poll functions, IMO.

 Also note that I've found that there is a similar bug in mvsata(4);
 mvsata_bio_poll() should be iterated until ATA_ITSDONE is set:

 http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ic/mvsata.c#rev1.36

 I've fixed it, but I guess that this does not fix weird behaviors
 reported for mvsata(4)...

 I will commit the patch if there's no objection.

 Thoughts?

 Thanks,
 rin

From: =?UTF-8?B?SmFyb23DrXIgRG9sZcSNZWs=?= <jaromir.dolecek@gmail.com>
To: 
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>
Subject: Re: kern/56403 (memory mapped mode of wdc(4) at pcmcia(4) broken
 after jdolecek-ncq branch merge)
Date: Mon, 4 Oct 2021 20:26:59 +0200

 Le lun. 4 oct. 2021 =C3=A0 08:57, Rin Okuyama <rokuyama.rk@gmail.com> a =C3=
 =A9crit :
 > I've found it!
 >
 > Before jdolecek-ncq merge, for ATA_POLL, _wdc_ata_bio_start() iterated
 > itself and wdc_ata_bio_intr(), until ATA_ITSDONE is achieved.

 Go ahead, thanks.

 I have only one concern, there is nothing stopping it looping forever.
 Maybe add a break condition of some sort?

 Jaromir

From: Rin Okuyama <rokuyama.rk@gmail.com>
To: gnats-bugs@netbsd.org, =?UTF-8?B?SmFyb23DrXIgRG9sZcSNZWs=?=
 <jaromir.dolecek@gmail.com>
Cc: 
Subject: Re: kern/56403 (memory mapped mode of wdc(4) at pcmcia(4) broken
 after jdolecek-ncq branch merge)
Date: Tue, 5 Oct 2021 16:29:00 +0900

 On 2021/10/05 3:30, Jaromír Doleček wrote:
 >   Go ahead, thanks.

 Thank you very much for your confirmation!

 >   I have only one concern, there is nothing stopping it looping forever.
 >   Maybe add a break condition of some sort?

 I don't think that this changes cause infinite recursion:

 (1) wdc_ata_bio_intr() called from c_poll (= wdc_ata_bio_poll()) asserts
      ATA_ITSDONE in its error handling paths via wdc_ata_bio_done().

 (2) Also, return value from c_start (= wdc_ata_bio_sart()) is checked in
      ata_xfer_start().

 So, errors encountered in c_poll and c_start routines should be handled
 appropriately. The situation is similar for mvsata(4).

 Still, there is a possibility where ata_xfer_start() takes long time to
 finish a normal operation. This can result in bad response for lower
 priority interrupts, etc.. But, I observe no such a situation, even if
 heavy thrashing takes place for swap partition in wd(4) (GCC requires
 more than 100MB virtual memory when compiling perl, even though my hpcsh
 machine has only 32MB physical memory!!).

 Thanks,
 rin

From: "Rin Okuyama" <rin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56403 CVS commit: src/sys/dev
Date: Tue, 5 Oct 2021 08:01:06 +0000

 Module Name:	src
 Committed By:	rin
 Date:		Tue Oct  5 08:01:05 UTC 2021

 Modified Files:
 	src/sys/dev/ata: ata.c ata_wdc.c atavar.h
 	src/sys/dev/ic: ahcisata_core.c mvsata.c siisata.c wdc.c
 	src/sys/dev/scsipi: atapi_wdc.c

 Log Message:
 PR kern/56403

 Fix kernel freeze for wdc(4) variants with ATAC_CAP_NOIRQ:

 (1) Change ata_xfer_ops:c_poll from void to int function. When it returns
     ATAPOLL_AGAIN, let ata_xfer_start() iterate itself again.

 (2) Let wdc_ata_bio_poll() return ATAPOLL_AGAIN until ATA_ITSDONE is
     achieved.

 A similar change has been made for mvsata(4) (see mvsata_bio_poll()),
 and no functional changes for other devices.

 This is how the drivers worked before jdolecek-ncq branch was merged.

 Note that this changes are less likely to cause infinite recursion:

 (1) wdc_ata_bio_intr() called from wdc_ata_bio_poll() asserts ATA_ITSDONE
     in its error handling paths via wdc_ata_bio_done().

 (2) Return value from c_start (= wdc_ata_bio_start()) is checked in
     ata_xfer_start().

 Therefore, errors encountered in ata_xfer_ops:c_poll and c_start routines
 terminate the recursion for wdc(4). The situation is similar for mvsata(4).

 Still, there is a possibility where ata_xfer_start() takes long time to
 finish a normal operation. This can result in a delayed response for lower
 priority interrupts. But, I've never observed such a situation, even when
 heavy thrashing takes place for swap partition in wd(4).

 "Go ahead" by jdolecek@.


 To generate a diff of this commit:
 cvs rdiff -u -r1.163 -r1.164 src/sys/dev/ata/ata.c
 cvs rdiff -u -r1.119 -r1.120 src/sys/dev/ata/ata_wdc.c
 cvs rdiff -u -r1.108 -r1.109 src/sys/dev/ata/atavar.h
 cvs rdiff -u -r1.101 -r1.102 src/sys/dev/ic/ahcisata_core.c
 cvs rdiff -u -r1.60 -r1.61 src/sys/dev/ic/mvsata.c
 cvs rdiff -u -r1.48 -r1.49 src/sys/dev/ic/siisata.c
 cvs rdiff -u -r1.307 -r1.308 src/sys/dev/ic/wdc.c
 cvs rdiff -u -r1.140 -r1.141 src/sys/dev/scsipi/atapi_wdc.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56403 CVS commit: [netbsd-9] src/sys/dev
Date: Fri, 30 Dec 2022 14:39:10 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Fri Dec 30 14:39:10 UTC 2022

 Modified Files:
 	src/sys/dev/ata [netbsd-9]: ata.c ata_recovery.c ata_wdc.c atavar.h
 	    satapmp_subr.c wd.c
 	src/sys/dev/ic [netbsd-9]: ahcisata_core.c mvsata.c siisata.c wdc.c
 	    wdcvar.h
 	src/sys/dev/scsipi [netbsd-9]: atapi_wdc.c
 	src/sys/dev/usb [netbsd-9]: umass_isdata.c

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #1557):

 	sys/dev/ic/ahcisata_core.c: revision 1.83
 	sys/dev/ic/ahcisata_core.c: revision 1.102
 	sys/dev/ata/ata.c: revision 1.164
 	sys/dev/ata/ata_wdc.c: revision 1.115
 	sys/dev/ata/ata_recovery.c: revision 1.4
 	sys/dev/ic/siisata.c: revision 1.42
 	sys/dev/ic/wdc.c: revision 1.308
 	sys/dev/ic/mvsata.c: revision 1.56
 	sys/dev/scsipi/atapi_wdc.c: revision 1.138
 	sys/dev/ic/siisata.c: revision 1.49
 	sys/dev/ata/atavar.h: revision 1.105
 	sys/dev/ata/wd.c: revision 1.460
 	sys/dev/ata/ata.c: revision 1.155
 	sys/dev/ata/wd.c: revision 1.462
 	sys/dev/ata/atavar.h: revision 1.109
 	sys/dev/ata/satapmp_subr.c: revision 1.16
 	sys/dev/ic/wdc.c: revision 1.299
 	sys/dev/ic/ahcisata_core.c: revision 1.93
 	sys/dev/ata/ata_wdc.c: revision 1.120
 	sys/dev/ic/wdcvar.h: revision 1.100
 	sys/dev/scsipi/atapi_wdc.c: revision 1.141
 	sys/dev/ic/mvsata.c: revision 1.61
 	sys/dev/usb/umass_isdata.c	(apply patch)

 drop wd lock in wdstart1() before calling the ata_bio hook; when called
 from ata thread context, that can still need to sleep for wdc attachments
 in wdcwait()

 fix use-after-free for ata xfer on bio submission found by KASAN
 driver ata_bio hooks read parts of the xfer after ata_exec_xfer()
 call in order to determine return value, change so that the hook
 doesn't return any value - callers do not care already,
 as all I/O requests are asynchronous

 this problem was uncovered by recent change for wd(4) to not hold
 wd mutex during ata_bio call, the interrupt for the xfer might
 thus actually fire immediately

 adjust also ata_exec_command driver hooks similarily - remove all
 completion and waiting logic from drivers, upper layer ata code
 using AT_WAIT/AT_POLL changed to call ata_wait_cmd() itself
 PR kern/55169 by Nick Hudson

 Function declaration formating whitespace consistency.  NFCI.

 PR kern/56403
 Fix kernel freeze for wdc(4) variants with ATAC_CAP_NOIRQ:

 (1) Change ata_xfer_ops:c_poll from void to int function. When it returns
     ATAPOLL_AGAIN, let ata_xfer_start() iterate itself again.
 (2) Let wdc_ata_bio_poll() return ATAPOLL_AGAIN until ATA_ITSDONE is
     achieved.

 A similar change has been made for mvsata(4) (see mvsata_bio_poll()),
 and no functional changes for other devices.

 This is how the drivers worked before jdolecek-ncq branch was merged.
 Note that this changes are less likely to cause infinite recursion:

 (1) wdc_ata_bio_intr() called from wdc_ata_bio_poll() asserts ATA_ITSDONE
     in its error handling paths via wdc_ata_bio_done().
 (2) Return value from c_start (= wdc_ata_bio_start()) is checked in
     ata_xfer_start().

 Therefore, errors encountered in ata_xfer_ops:c_poll and c_start routines
 terminate the recursion for wdc(4). The situation is similar for mvsata(4).

 Still, there is a possibility where ata_xfer_start() takes long time to
 finish a normal operation. This can result in a delayed response for lower
 priority interrupts. But, I've never observed such a situation, even when
 heavy thrashing takes place for swap partition in wd(4).
 "Go ahead" by jdolecek@.


 To generate a diff of this commit:
 cvs rdiff -u -r1.149.2.2 -r1.149.2.3 src/sys/dev/ata/ata.c
 cvs rdiff -u -r1.2.8.1 -r1.2.8.2 src/sys/dev/ata/ata_recovery.c
 cvs rdiff -u -r1.113 -r1.113.4.1 src/sys/dev/ata/ata_wdc.c
 cvs rdiff -u -r1.103 -r1.103.4.1 src/sys/dev/ata/atavar.h
 cvs rdiff -u -r1.15 -r1.15.4.1 src/sys/dev/ata/satapmp_subr.c
 cvs rdiff -u -r1.452.2.2 -r1.452.2.3 src/sys/dev/ata/wd.c
 cvs rdiff -u -r1.75.4.4 -r1.75.4.5 src/sys/dev/ic/ahcisata_core.c
 cvs rdiff -u -r1.48 -r1.48.2.1 src/sys/dev/ic/mvsata.c
 cvs rdiff -u -r1.39 -r1.39.4.1 src/sys/dev/ic/siisata.c
 cvs rdiff -u -r1.291.4.1 -r1.291.4.2 src/sys/dev/ic/wdc.c
 cvs rdiff -u -r1.98.10.1 -r1.98.10.2 src/sys/dev/ic/wdcvar.h
 cvs rdiff -u -r1.133 -r1.133.4.1 src/sys/dev/scsipi/atapi_wdc.c
 cvs rdiff -u -r1.42 -r1.42.4.1 src/sys/dev/usb/umass_isdata.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: rin@NetBSD.org
State-Changed-When: Fri, 21 Jul 2023 08:51:33 +0000
State-Changed-Why:
Fix will be in NetBSD 10.0. Pulled up to netbsd-9 also.


>Unformatted:

Home	PR Database Search