NetBSD Problem Report #54538
From tsutsui@ceres.dti.ne.jp Mon Sep 9 12:35:58 2019
Return-Path: <tsutsui@ceres.dti.ne.jp>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-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 C5E057A1E9
for <gnats-bugs@gnats.NetBSD.org>; Mon, 9 Sep 2019 12:35:58 +0000 (UTC)
Message-Id: <201909091235.x89CZop8022500@ceres.dti.ne.jp>
Date: Mon, 9 Sep 2019 21:35:50 +0900 (JST)
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Reply-To: tsutsui@ceres.dti.ne.jp
To: gnats-bugs@NetBSD.org
Cc: tsutsui@ceres.dti.ne.jp
Subject: wdc(4) regression after SATA NCQ merge
X-Send-Pr-Version: 3.95
>Number: 54538
>Category: kern
>Synopsis: wdc(4) regression after SATA NCQ merge
>Confidential: no
>Severity: critical
>Priority: low
>Responsible: tsutsui
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Mon Sep 09 12:40:00 +0000 2019
>Closed-Date: Mon Sep 23 15:54:22 +0000 2019
>Last-Modified: Mon Sep 23 15:54:22 +0000 2019
>Originator: Izumi Tsutsui
>Release: NetBSD 9.0_BETA updated around 201909091200
>Organization:
>Environment:
System: NetBSD 9.0_BETA (-current is also affected)
Architecture: all
Machine: all
>Description:
On SATA-NCQ (jdolecek-ncq branch) merge,
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ic/wdc.c#rev1.284
wdcprobe() function in src/sys/dev/ic/wd.c has been changed to take
(struct wdc_regs *wdr) instead of the previous (struct ata_channel *chp):
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ic/wdc.c.diff?r1=1.283&r2=1.284
---before---
int
wdcprobe(struct ata_channel *chp)
{
struct wdc_softc *wdc = CHAN_TO_WDC(chp);
/* default reset method */
if (wdc->reset == NULL)
wdc->reset = wdc_do_reset;
return (wdcprobe1(chp, 1));
}
---end of quote----
---after---
int
wdcprobe(struct wdc_regs *wdr)
{
struct wdc_softc wdc;
struct ata_channel ch;
int rv;
memset(&wdc, 0, sizeof(wdc));
memset(&ch, 0, sizeof(ch));
ata_channel_init(&ch);
ch.ch_atac = &wdc.sc_atac;
ch.ch_queue = ata_queue_alloc(1);
wdc.regs = wdr;
/* default reset method */
if (wdc.reset == NULL)
wdc.reset = wdc_do_reset;
rv = wdcprobe1(&ch, 1);
ata_queue_free(ch.ch_queue);
ata_channel_destroy(&ch);
return rv;
}
---end of quote---
As you can see above, we no longer can specify MD wdc reset function
because the struct wdc is prepared in wdcprobe() itself:
>> memset(&wdc, 0, sizeof(wdc));
:
>> /* default reset method */
>> if (wdc.reset == NULL)
>> wdc.reset = wdc_do_reset;
so currently there is no way to pass the MD function that was passed
via struct ata_channel.
This breaks at least sys/arch/dreamcast/conf/G1IDE functions.
(though not enabled by default)
Note there are more MD drivers that have not been updated
for the wdcprobe() API changes:
https://nxr.netbsd.org/source/s?refs=wdcprobe&project=src
/src/sys/arch/evbppc/mpc85xx/
wdc_obio.c 129 rv = wdcprobe(&ch);
/src/sys/arch/dreamcast/dev/g1/
wdc_g1.c 117 result = wdcprobe(&ch);
/src/sys/arch/mips/adm5120/dev/
wdc_extio.c 266 result = wdcprobe(&ch);
/src/sys/arch/mmeye/dev/
wdc_mainbus.c 118 result = wdcprobe(&ch);
>How-To-Repeat:
Code inspection.
>Fix:
1) Change wdcprobe() to take a MD reset function?
(I'm not sure how many attachments should be updated though)
2) Add a new public probe function (like wdcprobe2())
that also takes MD reset function?
3) Revert the wdcprobe() API change?
---
Izumi Tsutsui
>Release-Note:
>Audit-Trail:
Responsible-Changed-From-To: kern-bug-people->jdolecek
Responsible-Changed-By: jdolecek@NetBSD.org
Responsible-Changed-When: Mon, 09 Sep 2019 20:21:04 +0000
Responsible-Changed-Why:
I have to fix what I broke.
State-Changed-From-To: open->feedback
State-Changed-By: jdolecek@NetBSD.org
State-Changed-When: Mon, 09 Sep 2019 22:04:49 +0000
State-Changed-Why:
Committed a fix. Can you confirm it still works after change? The dreamcast
wdc reset didn't seem useful so I just disabled it.
From: "Jaromir Dolecek" <jdolecek@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54538 CVS commit: src/sys/arch
Date: Mon, 9 Sep 2019 22:01:24 +0000
Module Name: src
Committed By: jdolecek
Date: Mon Sep 9 22:01:23 UTC 2019
Modified Files:
src/sys/arch/dreamcast/dev/g1: wdc_g1.c
src/sys/arch/evbppc/mpc85xx: wdc_obio.c
src/sys/arch/mips/adm5120/dev: wdc_extio.c
src/sys/arch/mmeye/dev: wdc_mainbus.c
Log Message:
adjust several missed drivers for wdcprobe() changes of ATA NCQ branch
for dreamcast g1 just drop the custom reset function, it doesn't seem to do
anything useful over the generic variant
PR kern/54538 by Izumi Tsutsui
To generate a diff of this commit:
cvs rdiff -u -r1.3 -r1.4 src/sys/arch/dreamcast/dev/g1/wdc_g1.c
cvs rdiff -u -r1.6 -r1.7 src/sys/arch/evbppc/mpc85xx/wdc_obio.c
cvs rdiff -u -r1.10 -r1.11 src/sys/arch/mips/adm5120/dev/wdc_extio.c
cvs rdiff -u -r1.6 -r1.7 src/sys/arch/mmeye/dev/wdc_mainbus.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@netbsd.org
Cc: jdolecek@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: kern/54538 (wdc(4) regression after SATA NCQ merge)
Date: Tue, 10 Sep 2019 22:54:49 +0900
> Committed a fix. Can you confirm it still works after change? The dreamcast
> wdc reset didn't seem useful so I just disabled it.
It looks wdc_g1_do_reset() is not necessary for wdcprobe(),
but IIRC it was actually required on device accesses.
GD-ROM is a very early ATAPI devices (appeared in 1998) and
it doesn't reset its internal status by WDCTL_RST bit in AUXCTL
but requires ATAPI_SOFT_RESET command, as per
sys/arch/dreamcast/dev/gdrom.c.
Anyway I'll try if your fix works or not.
---
Izumi Tsutsui
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: gnats-bugs@netbsd.org
Cc: jdolecek@netbsd.org, tsutsui@ceres.dti.ne.jp
Subject: Re: kern/54538 (wdc(4) regression after SATA NCQ merge)
Date: Tue, 10 Sep 2019 23:49:04 +0900
> It looks wdc_g1_do_reset() is not necessary for wdcprobe(),
This is not correct.
It's also necessary even in wdc_g1_probe(), otherwise the driver
silently hangs in wdcprobe().
> Anyway I'll try if your fix works or not.
The following dumb patch (that restores MD wdc do_reset() function
support for wdcprobe()) makes dreamcast G1IDE functional.
https://dmesgd.nycbug.org/index.cgi?do=view&id=5115
Index: dev/ic/wdc.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/wdc.c,v
retrieving revision 1.291
diff -u -p -d -r1.291 wdc.c
--- dev/ic/wdc.c 27 Oct 2018 05:38:08 -0000 1.291
+++ dev/ic/wdc.c 10 Sep 2019 14:31:22 -0000
@@ -477,6 +477,14 @@ wdc_drvprobe(struct ata_channel *chp)
int
wdcprobe(struct wdc_regs *wdr)
{
+
+ return wdcprobe_with_reset(wdr, NULL);
+}
+
+int
+wdcprobe_with_reset(struct wdc_regs *wdr,
+ void (*do_reset)(struct ata_channel *, int))
+{
struct wdc_softc wdc;
struct ata_channel ch;
int rv;
@@ -487,9 +495,8 @@ wdcprobe(struct wdc_regs *wdr)
ch.ch_atac = &wdc.sc_atac;
wdc.regs = wdr;
- /* default reset method */
- if (wdc.reset == NULL)
- wdc.reset = wdc_do_reset;
+ /* check the MD reset method */
+ wdc.reset = (do_reset != NULL) ? do_reset : wdc_do_reset;
rv = wdcprobe1(&ch, 1);
Index: dev/ic/wdcvar.h
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/wdcvar.h,v
retrieving revision 1.98
diff -u -p -d -r1.98 wdcvar.h
--- dev/ic/wdcvar.h 7 Oct 2017 16:05:32 -0000 1.98
+++ dev/ic/wdcvar.h 10 Sep 2019 14:31:22 -0000
@@ -145,6 +145,8 @@ void wdc_allocate_regs(struct wdc_softc
void wdc_init_shadow_regs(struct wdc_regs *);
int wdcprobe(struct wdc_regs *);
+int wdcprobe_with_reset(struct wdc_regs *,
+ void (*)(struct ata_channel *, int));
void wdcattach(struct ata_channel *);
int wdcdetach(device_t, int);
void wdc_childdetached(device_t, device_t);
Index: arch/dreamcast/dev/g1/wdc_g1.c
===================================================================
RCS file: /cvsroot/src/sys/arch/dreamcast/dev/g1/wdc_g1.c,v
retrieving revision 1.4
diff -u -p -d -r1.4 wdc_g1.c
--- arch/dreamcast/dev/g1/wdc_g1.c 9 Sep 2019 22:01:23 -0000 1.4
+++ arch/dreamcast/dev/g1/wdc_g1.c 10 Sep 2019 14:31:22 -0000
@@ -62,9 +62,7 @@ struct wdc_g1_softc {
static int wdc_g1_probe(device_t, cfdata_t, void *);
static void wdc_g1_attach(device_t, device_t, void *);
-#if 0
static void wdc_g1_do_reset(struct ata_channel *, int);
-#endif
static int wdc_g1_intr(void *);
CFATTACH_DECL_NEW(wdc_g1bus, sizeof(struct wdc_g1_softc),
@@ -76,18 +74,11 @@ wdc_g1_probe(device_t parent, cfdata_t c
struct g1bus_attach_args *ga = aux;
struct wdc_regs wdr;
int result = 0, i;
-#ifdef ATADEBUG
- struct device dev;
-#endif
*((volatile uint32_t *)0xa05f74e4) = 0x1fffff;
for (i = 0; i < 0x200000 / 4; i++)
(void)((volatile uint32_t *)0xa0000000)[i];
-#if 0
- wdc.reset = wdc_g1_do_reset;
-#endif
-
wdr.cmd_iot = ga->ga_memt;
if (bus_space_map(wdr.cmd_iot, WDC_G1_CMD_ADDR,
WDC_G1_REG_NPORTS * 4, 0, &wdr.cmd_baseioh))
@@ -106,12 +97,7 @@ wdc_g1_probe(device_t parent, cfdata_t c
WDC_G1_AUXREG_NPORTS, 0, &wdr.ctl_ioh))
goto outunmap;
-#ifdef ATADEBUG
- /* fake up device name for ATADEBUG_PRINT() with DEBUG_PROBE */
- memset(&dev, 0, sizeof(dev));
- strncat(dev.dv_xname, "wdc(g1probe)", sizeof(dev.dv_xname));
-#endif
- result = wdcprobe(&wdr);
+ result = wdcprobe_with_reset(&wdr, wdc_g1_do_reset);
bus_space_unmap(wdr.ctl_iot, wdr.ctl_ioh, WDC_G1_AUXREG_NPORTS);
outunmap:
@@ -157,9 +143,7 @@ wdc_g1_attach(struct device *parent, str
sc->sc_wdcdev.sc_atac.atac_channels = sc->wdc_chanlist;
sc->sc_wdcdev.sc_atac.atac_nchannels = 1;
sc->sc_wdcdev.wdc_maxdrives = 2;
-#if 0
sc->sc_wdcdev.reset = wdc_g1_do_reset;
-#endif
sc->ata_channel.ch_channel = 0;
sc->ata_channel.ch_atac = &sc->sc_wdcdev.sc_atac;
@@ -180,12 +164,11 @@ wdc_g1_intr(void *arg)
return wdcintr(arg);
}
-#if 0
/*
- * This does what the generic wdc_do_reset() does, only with unnecessary
- * additional GD-ROM reset. Keep code around in case this turns out to be
- * actually useful/necessary. ATAPI code should do it's own reset in either
- * case anyway.
+ * This does what the generic wdc_do_reset() does, with additional
+ * GD-ROM reset. GD-ROM is a very early ATAPI device appeared in 1998
+ * and it doesn't reset itself by the WDCTL_RST in AUX_CTLR but requires
+ * ATAPI_SOFT_RESET command to reset whole device as a master.
*/
static void
wdc_g1_do_reset(struct ata_channel *chp, int poll)
@@ -220,4 +203,3 @@ wdc_g1_do_reset(struct ata_channel *chp,
if (poll != 0)
splx(s);
}
-#endif
---
Note ATADEBUG part in wdc_g1.c is no longer necessary after the
following MI wdc.c fix:
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ic/wdc.c#rev1.280
>> Fix a bug that wdcprobe1() accesses NULL pointer when the DEBUG_PROBE bit
>> is set in atadebug_mask variable. The caller passes data which has
>> temporary-generated wdc_softc in it, but the device_t has not initialized
>> because it's not determined yet. So it can't use device_xname(). Use
>> __function__ instead.
---
Izumi Tsutsui
From: =?UTF-8?B?SmFyb23DrXIgRG9sZcSNZWs=?= <jaromir.dolecek@gmail.com>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: "gnats-bugs@NetBSD.org" <gnats-bugs@netbsd.org>
Subject: Re: kern/54538 (wdc(4) regression after SATA NCQ merge)
Date: Tue, 10 Sep 2019 19:49:59 +0200
Le mar. 10 sept. 2019 =C3=A0 16:49, Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>=
a =C3=A9crit :
> It's also necessary even in wdc_g1_probe(), otherwise the driver
> silently hangs in wdcprobe().
>
> > Anyway I'll try if your fix works or not.
>
> The following dumb patch (that restores MD wdc do_reset() function
> support for wdcprobe()) makes dreamcast G1IDE functional.
> https://dmesgd.nycbug.org/index.cgi?do=3Dview&id=3D5115
Allright, please commit this - it's good enough.
From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54538 CVS commit: src/sys
Date: Sat, 14 Sep 2019 17:11:39 +0000
Module Name: src
Committed By: tsutsui
Date: Sat Sep 14 17:11:39 UTC 2019
Modified Files:
src/sys/arch/dreamcast/dev/g1: wdc_g1.c
src/sys/dev/ic: wdc.c wdcvar.h
Log Message:
Restore interface to pass a MD reset function to MI wdcprobe().
Fixes silent hang on G1IDE on Dreamcast. PR kern/54538
Should be pulled up to netbsd-9 with the previous changes.
To generate a diff of this commit:
cvs rdiff -u -r1.4 -r1.5 src/sys/arch/dreamcast/dev/g1/wdc_g1.c
cvs rdiff -u -r1.291 -r1.292 src/sys/dev/ic/wdc.c
cvs rdiff -u -r1.98 -r1.99 src/sys/dev/ic/wdcvar.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Responsible-Changed-From-To: jdolecek->tsutsui
Responsible-Changed-By: tsutsui@NetBSD.org
Responsible-Changed-When: Mon, 23 Sep 2019 00:50:36 +0000
Responsible-Changed-Why:
Fxied in HEAD and I send a pullup request for netbsd-9.
State-Changed-From-To: feedback->pending-pullups
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Mon, 23 Sep 2019 00:50:36 +0000
State-Changed-Why:
[pullup-9 #232]
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/54538 CVS commit: [netbsd-9] src/sys
Date: Mon, 23 Sep 2019 07:09:47 +0000
Module Name: src
Committed By: martin
Date: Mon Sep 23 07:09:47 UTC 2019
Modified Files:
src/sys/arch/dreamcast/dev/g1 [netbsd-9]: wdc_g1.c
src/sys/arch/evbppc/mpc85xx [netbsd-9]: wdc_obio.c
src/sys/arch/mips/adm5120/dev [netbsd-9]: wdc_extio.c
src/sys/arch/mmeye/dev [netbsd-9]: wdc_mainbus.c
src/sys/dev/ic [netbsd-9]: wdc.c wdcvar.h
Log Message:
Pull up following revision(s) (requested by tsutsui in ticket #232):
sys/arch/evbppc/mpc85xx/wdc_obio.c: revision 1.7
sys/arch/dreamcast/dev/g1/wdc_g1.c: revision 1.4
sys/arch/dreamcast/dev/g1/wdc_g1.c: revision 1.5
sys/arch/mmeye/dev/wdc_mainbus.c: revision 1.7
sys/dev/ic/wdcvar.h: revision 1.99
sys/dev/ic/wdc.c: revision 1.292
sys/arch/mips/adm5120/dev/wdc_extio.c: revision 1.11
adjust several missed drivers for wdcprobe() changes of ATA NCQ branch
for dreamcast g1 just drop the custom reset function, it doesn't seem to do
anything useful over the generic variant
PR kern/54538 by Izumi Tsutsui
Restore interface to pass a MD reset function to MI wdcprobe().
Fixes silent hang on G1IDE on Dreamcast. PR kern/54538
Should be pulled up to netbsd-9 with the previous changes.
To generate a diff of this commit:
cvs rdiff -u -r1.3 -r1.3.10.1 src/sys/arch/dreamcast/dev/g1/wdc_g1.c
cvs rdiff -u -r1.6 -r1.6.8.1 src/sys/arch/evbppc/mpc85xx/wdc_obio.c
cvs rdiff -u -r1.10 -r1.10.8.1 src/sys/arch/mips/adm5120/dev/wdc_extio.c
cvs rdiff -u -r1.6 -r1.6.8.1 src/sys/arch/mmeye/dev/wdc_mainbus.c
cvs rdiff -u -r1.291 -r1.291.4.1 src/sys/dev/ic/wdc.c
cvs rdiff -u -r1.98 -r1.98.10.1 src/sys/dev/ic/wdcvar.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: pending-pullups->closed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Mon, 23 Sep 2019 15:54:22 +0000
State-Changed-Why:
Pullup complete.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home