From: Michael Kelley <redacted>
Sent: Friday, August 23, 2019 1:17 PM

From: Dexuan Cui Sent: Monday, August 19, 2019 6:52 PM

quoted

diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c

@@ -499,6 +499,8 @@ static void vmbus_add_channel_work(struct

work_struct *work)

quoted

 	return;

 err_deq_chan:
+	WARN_ON_ONCE(1);
+

Why this change?  I was thinking maybe it's a debug statement that got
left in.

This is indeed a debug statement. :-) 
I was not so sure if the error handling is absolutely correct there.
I think I can remove this WARN_ON_ONCE() since almost all of the possible
error paths have a pr_err(). We can make an extra patch to fix any bug in
the existing error handling code. BTW, it should be pretty unlikely to reach
the "err_deq_chan label" in practice.

quoted

@@ -1021,6 +1044,11 @@ static void vmbus_onoffer_rescind(struct

vmbus_channel_message_header *hdr)
 		}
 		mutex_unlock(&vmbus_connection.channel_mutex);
 	}
+
+	/* The "channel" may have been freed. Do not access it any longer. */
+
+	if (clean_up_chan_for_suspend)
+		check_ready_for_suspend_event();
 }

Having to add the above lines twice is a bit clumsy, but the problem is
the overall structure of the vmbus_onoffer_rescind.  The early return in
the case of a rescind_callback function is a bit weird, but I guess it makes
sense since from what I can tell, only uio and hv_sock have rescind callback
functions. Some minor restructuring might be warranted, but I don't feel
strongly about it.

I agree. We should restructure vmbus_onoffer_rescind(), but I think we can
do it in a separate patch. Here in this patch I'm focused on the minimal
required change for hibernation.

quoted

+	/*
+	 * Wait until all the sub-channels and hv_sock channels have been
+	 * cleaned up. Sub-channels should be destroyed upon suspend,

otherwise

quoted

+	 * they would conflict with the new sub-channels that will be created
+	 * in the resume path. hv_sock channels should also be destroyed, but
+	 * a hv_sock channel of an established hv_sock connection can not be
+	 * really destroyed since it may still be referenced by the userspace
+	 * application, so we just force the hv_sock channel to be rescinded
+	 * by vmbus_force_channel_rescinded(), and the userspace application
+	 * will thoroughly destroy the channel after hibernation.
+	 */
+	if (atomic_read(&vmbus_connection.nr_chan_close_on_suspend) > 0)

At first glance, the above line seemed useless to me.  You could just do the
wait_for_completion() unconditionally.  But is the intent to handle the case
where the VM never had any sub-channels or hv_sock channels, and so
nr_chan_close_on_suspend never went above 0?  

Yes.

If so, a comment might be helpful.

quoted

+wait_for_completion(&vmbus_connection.ready_for_suspend_event);

Ok. I'll add a commnt for this in v4.

Thanks,
-- Dexuan