NetBSD-Bugs archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: kern/43125 (zyd(4) crashes with Buffalo WLI-U2-KG54L)



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

From: Izumi Tsutsui <tsutsui%ceres.dti.ne.jp@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: mlelstv%NetBSD.org@localhost, kern-bug-people%NetBSD.org@localhost, 
netbsd-bugs%NetBSD.org@localhost,
        gnats-admin%NetBSD.org@localhost, mlelstv%NetBSD.org@localhost, 
gson%gson.org@localhost,
        tsutsui%ceres.dti.ne.jp@localhost
Subject: Re: kern/43125 (zyd(4) crashes with Buffalo WLI-U2-KG54L)
Date: Sun, 13 Jun 2010 12:31:25 +0900

 > Synopsis: zyd(4) crashes with Buffalo WLI-U2-KG54L
 
 > I can repeat the problem with a SMC USB2.0 WLAN and have a patch.
 
 Any progress on this?
 
 > State-Changed-From-To: open->analyzed
  :
 > zyd (and probably rum) do some magic to delay firmware installation
 > until the device is initialized. However, you can call ioctl which
 > then uses uninitialized data structures.
 
 FUKAUMI Naoki suggested on tech-kern@ to introduce aftermountroothook()
 to handle deferred initialization that should be done after mountroot():
 http://mail-index.NetBSD.org/tech-kern/2010/05/27/msg008261.html
 
 But as I wrote in the thread, I think it's rather better to handle
 such device initializations in the autoconf(9) layer as we do
 in config_interrupt(9) than adding a common hook because
 such MI hooks seem deprecated like shutdownhook_establish(9) etc.
 
 How about this patch that introduces config_mountroot(9)?
 (zyd diff is based on FUKAUMI-san's one)
 
 ---
 Index: dev/usb/if_zyd.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/usb/if_zyd.c,v
 retrieving revision 1.25
 diff -u -r1.25 if_zyd.c
 --- dev/usb/if_zyd.c   5 Apr 2010 07:21:49 -0000       1.25
 +++ dev/usb/if_zyd.c   13 Jun 2010 03:00:12 -0000
 @@ -156,7 +156,7 @@
  CFATTACH_DECL_NEW(zyd, sizeof(struct zyd_softc), zyd_match,
      zyd_attach, zyd_detach, zyd_activate);
  
 -Static int    zyd_attachhook(void *);
 +Static void   zyd_attachhook(device_t);
  Static int    zyd_complete_attach(struct zyd_softc *);
  Static int    zyd_open_pipes(struct zyd_softc *);
  Static void   zyd_close_pipes(struct zyd_softc *);
 @@ -245,10 +245,10 @@
            UMATCH_VENDOR_PRODUCT : UMATCH_NONE;
  }
  
 -Static int
 -zyd_attachhook(void *xsc)
 +Static void
 +zyd_attachhook(device_t self)
  {
 -      struct zyd_softc *sc = xsc;
 +      struct zyd_softc *sc = device_private(self);
        firmware_handle_t fwh;
        const char *fwname;
        u_char *fw;
 @@ -259,7 +259,7 @@
        if ((error = firmware_open("zyd", fwname, &fwh)) != 0) {
                aprint_error_dev(sc->sc_dev,
                    "failed to open firmware %s (error=%d)\n", fwname, error);
 -              return error;
 +              return;
        }
        size = firmware_get_size(fwh);
        fw = firmware_malloc(size);
 @@ -267,7 +267,7 @@
                aprint_error_dev(sc->sc_dev,
                    "failed to allocate firmware memory\n");
                firmware_close(fwh);
 -              return ENOMEM;
 +              return;
        }
        error = firmware_read(fwh, 0, fw, size);
        firmware_close(fwh);
 @@ -275,7 +275,7 @@
                aprint_error_dev(sc->sc_dev,
                    "failed to read firmware (error %d)\n", error);
                firmware_free(fw, 0);
 -              return error;
 +              return;
        }
  
        error = zyd_loadfirmware(sc, fw, size);
 @@ -283,7 +283,7 @@
                aprint_error_dev(sc->sc_dev,
                    "could not load firmware (error=%d)\n", error);
                firmware_free(fw, 0);
 -              return ENXIO;
 +              return;
        }
  
        firmware_free(fw, 0);
 @@ -292,7 +292,7 @@
        /* complete the attach process */
        if ((error = zyd_complete_attach(sc)) == 0)
                sc->attached = 1;
 -      return error;
 +      return;
  }
  
  void
 @@ -334,13 +334,10 @@
        IFQ_SET_READY(&ifp->if_snd);
        memcpy(ifp->if_xname, device_xname(sc->sc_dev), IFNAMSIZ);
  
 -      if_attach(ifp);
 -      /* XXXX: alloc temporarily until the layer2 can be configured. */
 -      if_alloc_sadl(ifp);
 -
        SIMPLEQ_INIT(&sc->sc_rqh);
  
 -      return;
 +      /* defer configrations after file system is ready to load firmware */
 +      config_mountroot(self, zyd_attachhook);
  }
  
  Static int
 @@ -424,7 +421,7 @@
                    IEEE80211_CHAN_DYN | IEEE80211_CHAN_2GHZ;
        }
  
 -      if_free_sadl(ifp);
 +      if_attach(ifp);
        ieee80211_ifattach(ic);
        ic->ic_node_alloc = zyd_node_alloc;
        ic->ic_newassoc = zyd_newassoc;
 @@ -461,11 +458,8 @@
        struct ifnet *ifp = &sc->sc_if;
        int s;
  
 -      if (!sc->attached) {
 -              if_free_sadl(ifp);
 -              if_detach(ifp);
 +      if (!sc->attached)
                return 0;
 -      }
  
        s = splusb();
  
 @@ -2450,10 +2444,6 @@
        struct ieee80211com *ic = &sc->sc_ic;
        int i, error;
  
 -      if ((sc->sc_flags & ZD1211_FWLOADED) == 0)
 -              if ((error = zyd_attachhook(sc)) != 0)
 -                      return error;
 -
        zyd_stop(ifp, 0);
  
        IEEE80211_ADDR_COPY(ic->ic_myaddr, CLLADDR(ifp->if_sadl));
 Index: kern/init_main.c
 ===================================================================
 RCS file: /cvsroot/src/sys/kern/init_main.c,v
 retrieving revision 1.420
 diff -u -r1.420 init_main.c
 --- kern/init_main.c   10 Jun 2010 20:54:53 -0000      1.420
 +++ kern/init_main.c   13 Jun 2010 03:00:12 -0000
 @@ -266,6 +266,7 @@
  static void start_init(void *);
  static void configure(void);
  static void configure2(void);
 +static void configure3(void);
  void main(void);
  
  /*
 @@ -626,6 +627,8 @@
        } while (error != 0);
        mountroothook_destroy();
  
 +      configure3();
 +
        /*
         * Initialise the time-of-day clock, passing the time recorded
         * in the root filesystem (if any) for use by systems that
 @@ -783,6 +786,20 @@
  }
  
  static void
 +configure3(void)
 +{
 +
 +      /*
 +       * Create threads to call back and finish configuration for
 +       * devices that want the mounted root file system.
 +       */
 +      config_create_mountrootthreads();
 +
 +      /* Get the threads going and into any sleeps before continuing. */
 +      yield();
 +}
 +
 +static void
  check_console(struct lwp *l)
  {
        struct vnode *vp;
 Index: kern/subr_autoconf.c
 ===================================================================
 RCS file: /cvsroot/src/sys/kern/subr_autoconf.c,v
 retrieving revision 1.206
 diff -u -r1.206 subr_autoconf.c
 --- kern/subr_autoconf.c       30 Apr 2010 21:17:22 -0000      1.206
 +++ kern/subr_autoconf.c       13 Jun 2010 03:00:12 -0000
 @@ -200,6 +200,9 @@
  struct deferred_config_head interrupt_config_queue =
        TAILQ_HEAD_INITIALIZER(interrupt_config_queue);
  int interrupt_config_threads = 8;
 +struct deferred_config_head mountroot_config_queue =
 +      TAILQ_HEAD_INITIALIZER(mountroot_config_queue);
 +int mountroot_config_threads = 2;
  
  static void config_process_deferred(struct deferred_config_head *, device_t);
  
 @@ -427,6 +430,7 @@
  {
        config_process_deferred(&deferred_config_queue, dev);
        config_process_deferred(&interrupt_config_queue, dev);
 +      config_process_deferred(&mountroot_config_queue, dev);
  }
  
  static void
 @@ -454,6 +458,30 @@
        }
  }
  
 +static void
 +config_mountroot_thread(void *cookie)
 +{
 +      struct deferred_config *dc;
 +
 +      while ((dc = TAILQ_FIRST(&mountroot_config_queue)) != NULL) {
 +              TAILQ_REMOVE(&mountroot_config_queue, dc, dc_queue);
 +              (*dc->dc_func)(dc->dc_dev);
 +              kmem_free(dc, sizeof(*dc));
 +      }
 +      kthread_exit(0);
 +}
 +
 +void
 +config_create_mountrootthreads()
 +{
 +      int i;
 +
 +      for (i = 0; i < mountroot_config_threads; i++) {
 +              (void)kthread_create(PRI_NONE, 0, NULL,
 +                  config_mountroot_thread, NULL, NULL, "config");
 +      }
 +}
 +
  /*
   * Announce device attach/detach to userland listeners.
   */
 @@ -1848,6 +1876,39 @@
  }
  
  /*
 + * Defer some autoconfiguration for a device until after root file system
 + * is mounted (to load firmware etc).
 + */
 +void
 +config_mountroot(device_t dev, void (*func)(device_t))
 +{
 +      struct deferred_config *dc;
 +
 +      /*
 +       * If root file system is mounted, callback now.
 +       */
 +      if (rootvnode != NULL) {
 +              (*func)(dev);
 +              return;
 +      }
 +
 +#ifdef DIAGNOSTIC
 +      TAILQ_FOREACH(dc, &mountroot_config_queue, dc_queue) {
 +              if (dc->dc_dev == dev)
 +                      panic("%s: deferred twice", __func__);
 +      }
 +#endif
 +
 +      dc = kmem_alloc(sizeof(*dc), KM_SLEEP);
 +      if (dc == NULL)
 +              panic("%s: unable to allocate callback", __func__);
 +
 +      dc->dc_dev = dev;
 +      dc->dc_func = func;
 +      TAILQ_INSERT_TAIL(&mountroot_config_queue, dc, dc_queue);
 +}
 +
 +/*
   * Process a deferred configuration queue.
   */
  static void
 Index: sys/device.h
 ===================================================================
 RCS file: /cvsroot/src/sys/sys/device.h,v
 retrieving revision 1.136
 diff -u -r1.136 device.h
 --- sys/device.h       25 Mar 2010 19:23:18 -0000      1.136
 +++ sys/device.h       13 Jun 2010 03:00:15 -0000
 @@ -479,9 +479,11 @@
  void  config_defer(device_t, void (*)(device_t));
  void  config_deferred(device_t);
  void  config_interrupts(device_t, void (*)(device_t));
 +void  config_mountroot(device_t, void (*)(device_t));
  void  config_pending_incr(void);
  void  config_pending_decr(void);
  void  config_create_interruptthreads(void);
 +void  config_create_mountrootthreads(void);
  
  int   config_finalize_register(device_t, int (*)(device_t));
  void  config_finalize(void);
 
 ---
 
 upgt(4) (which requires firmware to get MAC address) on hpcarm
 works fine with this patch.
 
 (though I wonder if we should avoid such copy-n-pasted code..)
 
 ---
 Izumi Tsutsui
 


Home | Main Index | Thread Index | Old Index