Re: kern/54538 (wdc(4) regression after SATA NCQ merge)

[Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>]

The following reply was made to PR kern/54538; it has been noted by GNATS.

From: Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: jdolecek%netbsd.org@localhost, tsutsui%ceres.dti.ne.jp@localhost
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