Author: sephe
Date: Tue Oct 11 08:22:17 2016
New Revision: 307028
URL: https://svnweb.freebsd.org/changeset/base/307028

Log:
  MFC 302816-302818
  
  302816
      hyperv/vmbus: Release vmbus channel lock before detach devices
  
      Device detach method may sleep.
  
      While I'm here, rename the function, fix indentation and function
      comment.
  
      Sponsored by:   Microsoft OSTC
      Differential Revision:  https://reviews.freebsd.org/D7110
  
  302817
      hyperv/vmbus: Field renaming to reflect reality
  
      Sponsored by:   Microsoft OSTC
      Differential Revision:  https://reviews.freebsd.org/D7111
  
  302818
      hyperv/vmbus: Fix the racy channel close.
  
      It is not safe to iterate the sub-channel list w/o lock on the
      close path, while it's even more difficult to hold the lock
      and iterate the sub-channel list.  We leverage the
      vmbua_{get,rel}_subchan() functions to solve this dilemma.
  
      Sponsored by:   Microsoft OSTC
      Differential Revision:  https://reviews.freebsd.org/D7112

Modified:
  stable/10/sys/dev/hyperv/include/hyperv.h
  stable/10/sys/dev/hyperv/vmbus/hv_channel.c
  stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c
  stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h
  stable/10/sys/dev/hyperv/vmbus/vmbus.c
  stable/10/sys/dev/hyperv/vmbus/vmbus_var.h
Directory Properties:
  stable/10/   (props changed)

Modified: stable/10/sys/dev/hyperv/include/hyperv.h
==============================================================================
--- stable/10/sys/dev/hyperv/include/hyperv.h   Tue Oct 11 08:14:11 2016        
(r307027)
+++ stable/10/sys/dev/hyperv/include/hyperv.h   Tue Oct 11 08:22:17 2016        
(r307028)
@@ -326,7 +326,7 @@ typedef struct hv_vmbus_channel {
        void                            *hv_chan_priv3;
 
        struct task                     ch_detach_task;
-       TAILQ_ENTRY(hv_vmbus_channel)   ch_link;
+       TAILQ_ENTRY(hv_vmbus_channel)   ch_prilink;     /* primary chan link */
        uint32_t                        ch_subidx;      /* subchan index */
        volatile uint32_t               ch_stflags;     /* atomic-op */
                                                        /* VMBUS_CHAN_ST_ */

Modified: stable/10/sys/dev/hyperv/vmbus/hv_channel.c
==============================================================================
--- stable/10/sys/dev/hyperv/vmbus/hv_channel.c Tue Oct 11 08:14:11 2016        
(r307027)
+++ stable/10/sys/dev/hyperv/vmbus/hv_channel.c Tue Oct 11 08:22:17 2016        
(r307028)
@@ -542,35 +542,40 @@ hv_vmbus_channel_close_internal(hv_vmbus
            M_DEVBUF);
 }
 
-/**
- * @brief Close the specified channel
+/*
+ * Caller should make sure that all sub-channels have
+ * been added to 'chan' and all to-be-closed channels
+ * are not being opened.
  */
 void
-hv_vmbus_channel_close(hv_vmbus_channel *channel)
+hv_vmbus_channel_close(struct hv_vmbus_channel *chan)
 {
-       hv_vmbus_channel*       sub_channel;
+       int subchan_cnt;
 
-       if (channel->primary_channel != NULL) {
+       if (!VMBUS_CHAN_ISPRIMARY(chan)) {
                /*
-                * We only close multi-channels when the primary is
-                * closed.
+                * Sub-channel is closed when its primary channel
+                * is closed; done.
                 */
                return;
        }
 
        /*
-        * Close all multi-channels first.
+        * Close all sub-channels, if any.
         */
-       TAILQ_FOREACH(sub_channel, &channel->sc_list_anchor,
-           sc_list_entry) {
-               if ((sub_channel->ch_stflags & VMBUS_CHAN_ST_OPENED) == 0)
-                       continue;
-               hv_vmbus_channel_close_internal(sub_channel);
+       subchan_cnt = chan->subchan_cnt;
+       if (subchan_cnt > 0) {
+               struct hv_vmbus_channel **subchan;
+               int i;
+
+               subchan = vmbus_get_subchan(chan, subchan_cnt);
+               for (i = 0; i < subchan_cnt; ++i)
+                       hv_vmbus_channel_close_internal(subchan[i]);
+               vmbus_rel_subchan(subchan, subchan_cnt);
        }
-       /*
-        * Then close the primary channel.
-        */
-       hv_vmbus_channel_close_internal(channel);
+
+       /* Then close the primary channel. */
+       hv_vmbus_channel_close_internal(chan);
 }
 
 /**

Modified: stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c
==============================================================================
--- stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c    Tue Oct 11 08:14:11 
2016        (r307027)
+++ stable/10/sys/dev/hyperv/vmbus/hv_channel_mgmt.c    Tue Oct 11 08:22:17 
2016        (r307028)
@@ -137,8 +137,8 @@ vmbus_chan_add(struct hv_vmbus_channel *
                    newchan->ch_id, newchan->ch_subidx);
        }
 
-       mtx_lock(&sc->vmbus_chlist_lock);
-       TAILQ_FOREACH(prichan, &sc->vmbus_chlist, ch_link) {
+       mtx_lock(&sc->vmbus_prichan_lock);
+       TAILQ_FOREACH(prichan, &sc->vmbus_prichans, ch_prilink) {
                if (memcmp(&prichan->ch_guid_type, &newchan->ch_guid_type,
                    sizeof(struct hyperv_guid)) == 0 &&
                    memcmp(&prichan->ch_guid_inst, &newchan->ch_guid_inst,
@@ -148,18 +148,19 @@ vmbus_chan_add(struct hv_vmbus_channel *
        if (VMBUS_CHAN_ISPRIMARY(newchan)) {
                if (prichan == NULL) {
                        /* Install the new primary channel */
-                       TAILQ_INSERT_TAIL(&sc->vmbus_chlist, newchan, ch_link);
-                       mtx_unlock(&sc->vmbus_chlist_lock);
+                       TAILQ_INSERT_TAIL(&sc->vmbus_prichans, newchan,
+                           ch_prilink);
+                       mtx_unlock(&sc->vmbus_prichan_lock);
                        return 0;
                } else {
-                       mtx_unlock(&sc->vmbus_chlist_lock);
+                       mtx_unlock(&sc->vmbus_prichan_lock);
                        device_printf(sc->vmbus_dev, "duplicated primary "
                            "chan%u\n", newchan->ch_id);
                        return EINVAL;
                }
        } else { /* Sub-channel */
                if (prichan == NULL) {
-                       mtx_unlock(&sc->vmbus_chlist_lock);
+                       mtx_unlock(&sc->vmbus_prichan_lock);
                        device_printf(sc->vmbus_dev, "no primary chan for "
                            "chan%u\n", newchan->ch_id);
                        return EINVAL;
@@ -171,7 +172,7 @@ vmbus_chan_add(struct hv_vmbus_channel *
                 * XXX refcnt prichan
                 */
        }
-       mtx_unlock(&sc->vmbus_chlist_lock);
+       mtx_unlock(&sc->vmbus_prichan_lock);
 
        /*
         * This is a sub-channel; link it with the primary channel.
@@ -401,28 +402,28 @@ vmbus_channel_on_offers_delivered(struct
        vmbus_scan_done(sc);
 }
 
-/**
- * @brief Release channels that are unattached/unconnected (i.e., no drivers 
associated)
+/*
+ * Detach all devices and destroy the corresponding primary channels.
  */
 void
-hv_vmbus_release_unattached_channels(struct vmbus_softc *sc)
+vmbus_chan_destroy_all(struct vmbus_softc *sc)
 {
-       hv_vmbus_channel *channel;
+       struct hv_vmbus_channel *chan;
 
-       mtx_lock(&sc->vmbus_chlist_lock);
+       mtx_lock(&sc->vmbus_prichan_lock);
+       while ((chan = TAILQ_FIRST(&sc->vmbus_prichans)) != NULL) {
+               KASSERT(VMBUS_CHAN_ISPRIMARY(chan), ("not primary channel"));
+               TAILQ_REMOVE(&sc->vmbus_prichans, chan, ch_prilink);
+               mtx_unlock(&sc->vmbus_prichan_lock);
 
-       while (!TAILQ_EMPTY(&sc->vmbus_chlist)) {
-           channel = TAILQ_FIRST(&sc->vmbus_chlist);
-           KASSERT(VMBUS_CHAN_ISPRIMARY(channel), ("not primary channel"));
-           TAILQ_REMOVE(&sc->vmbus_chlist, channel, ch_link);
+               hv_vmbus_child_device_unregister(chan);
+               vmbus_chan_free(chan);
 
-           hv_vmbus_child_device_unregister(channel);
-           vmbus_chan_free(channel);
+               mtx_lock(&sc->vmbus_prichan_lock);
        }
        bzero(sc->vmbus_chmap,
            sizeof(struct hv_vmbus_channel *) * VMBUS_CHAN_MAX);
-
-       mtx_unlock(&sc->vmbus_chlist_lock);
+       mtx_unlock(&sc->vmbus_prichan_lock);
 }
 
 /**

Modified: stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h
==============================================================================
--- stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h      Tue Oct 11 08:14:11 
2016        (r307027)
+++ stable/10/sys/dev/hyperv/vmbus/hv_vmbus_priv.h      Tue Oct 11 08:22:17 
2016        (r307028)
@@ -146,9 +146,6 @@ void                        hv_ring_buffer_read_begin(
 uint32_t               hv_ring_buffer_read_end(
                                hv_vmbus_ring_buffer_info       *ring_info);
 
-void                   hv_vmbus_release_unattached_channels(
-                           struct vmbus_softc *);
-
 int                    hv_vmbus_child_device_register(
                                        struct hv_vmbus_channel *chan);
 int                    hv_vmbus_child_device_unregister(

Modified: stable/10/sys/dev/hyperv/vmbus/vmbus.c
==============================================================================
--- stable/10/sys/dev/hyperv/vmbus/vmbus.c      Tue Oct 11 08:14:11 2016        
(r307027)
+++ stable/10/sys/dev/hyperv/vmbus/vmbus.c      Tue Oct 11 08:22:17 2016        
(r307028)
@@ -1184,8 +1184,8 @@ vmbus_doattach(struct vmbus_softc *sc)
 
        mtx_init(&sc->vmbus_scan_lock, "vmbus scan", NULL, MTX_DEF);
        sc->vmbus_gpadl = VMBUS_GPADL_START;
-       mtx_init(&sc->vmbus_chlist_lock, "vmbus chlist", NULL, MTX_DEF);
-       TAILQ_INIT(&sc->vmbus_chlist);
+       mtx_init(&sc->vmbus_prichan_lock, "vmbus prichan", NULL, MTX_DEF);
+       TAILQ_INIT(&sc->vmbus_prichans);
        sc->vmbus_chmap = malloc(
            sizeof(struct hv_vmbus_channel *) * VMBUS_CHAN_MAX, M_DEVBUF,
            M_WAITOK | M_ZERO);
@@ -1256,7 +1256,7 @@ cleanup:
        }
        free(sc->vmbus_chmap, M_DEVBUF);
        mtx_destroy(&sc->vmbus_scan_lock);
-       mtx_destroy(&sc->vmbus_chlist_lock);
+       mtx_destroy(&sc->vmbus_prichan_lock);
 
        return (ret);
 }
@@ -1314,7 +1314,7 @@ vmbus_detach(device_t dev)
 {
        struct vmbus_softc *sc = device_get_softc(dev);
 
-       hv_vmbus_release_unattached_channels(sc);
+       vmbus_chan_destroy_all(sc);
 
        vmbus_disconnect(sc);
 
@@ -1333,7 +1333,7 @@ vmbus_detach(device_t dev)
 
        free(sc->vmbus_chmap, M_DEVBUF);
        mtx_destroy(&sc->vmbus_scan_lock);
-       mtx_destroy(&sc->vmbus_chlist_lock);
+       mtx_destroy(&sc->vmbus_prichan_lock);
 
        return (0);
 }

Modified: stable/10/sys/dev/hyperv/vmbus/vmbus_var.h
==============================================================================
--- stable/10/sys/dev/hyperv/vmbus/vmbus_var.h  Tue Oct 11 08:14:11 2016        
(r307027)
+++ stable/10/sys/dev/hyperv/vmbus/vmbus_var.h  Tue Oct 11 08:22:17 2016        
(r307028)
@@ -101,8 +101,9 @@ struct vmbus_softc {
 #define VMBUS_SCAN_CHCNT_DONE  0x80000000
        uint32_t                vmbus_scan_devcnt;
 
-       struct mtx              vmbus_chlist_lock;
-       TAILQ_HEAD(, hv_vmbus_channel) vmbus_chlist;
+       /* Primary channels */
+       struct mtx              vmbus_prichan_lock;
+       TAILQ_HEAD(, hv_vmbus_channel) vmbus_prichans;
 };
 
 #define VMBUS_FLAG_ATTACHED    0x0001  /* vmbus was attached */
@@ -137,6 +138,7 @@ void        vmbus_handle_intr(struct trapframe 
 void   vmbus_et_intr(struct trapframe *);
 
 void   vmbus_chan_msgproc(struct vmbus_softc *, const struct vmbus_message *);
+void   vmbus_chan_destroy_all(struct vmbus_softc *);
 
 struct vmbus_msghc *vmbus_msghc_get(struct vmbus_softc *, size_t);
 void   vmbus_msghc_put(struct vmbus_softc *, struct vmbus_msghc *);
_______________________________________________
svn-src-stable-10@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-stable-10
To unsubscribe, send any mail to "svn-src-stable-10-unsubscr...@freebsd.org"

Reply via email to