From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755885AbbIUFZf (ORCPT ); Mon, 21 Sep 2015 01:25:35 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:32798 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754182AbbIUFZe (ORCPT ); Mon, 21 Sep 2015 01:25:34 -0400 Date: Sun, 20 Sep 2015 22:25:32 -0700 From: Greg KH To: "K. Y. Srinivasan" Cc: linux-kernel@vger.kernel.org, devel@linuxdriverproject.org, olaf@aepfle.de, apw@canonical.com, vkuznets@redhat.com, jasowang@redhat.com Subject: Re: [PATCH 2/5] hv: add helpers to handle hv_util device state Message-ID: <20150921052532.GA24350@kroah.com> References: <1442363823-22428-1-git-send-email-kys@microsoft.com> <1442363874-22508-1-git-send-email-kys@microsoft.com> <1442363874-22508-2-git-send-email-kys@microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1442363874-22508-2-git-send-email-kys@microsoft.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 15, 2015 at 05:37:51PM -0700, K. Y. Srinivasan wrote: > From: Olaf Hering > > The callbacks in kvp, vss and fcopy code are called both from the main thread > as well as from interrupt context. If a state change is done by the main > thread it is not immediately seen by the interrupt. As a result the > state machine gets out of sync. > > Force propagation of state changes via get/set helpers with a memory barrier. > > Signed-off-by: Olaf Hering > Signed-off-by: K. Y. Srinivasan > --- > drivers/hv/hyperv_vmbus.h | 14 ++++++++++++++ > 1 files changed, 14 insertions(+), 0 deletions(-) > > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h > index 4b1eb6d..dee5798 100644 > --- a/drivers/hv/hyperv_vmbus.h > +++ b/drivers/hv/hyperv_vmbus.h > @@ -780,4 +780,18 @@ enum hvutil_device_state { > HVUTIL_DEVICE_DYING, /* driver unload is in progress */ > }; > > +static inline void hvutil_device_set_state(enum hvutil_device_state *p, > + enum hvutil_device_state s) > +{ > + *p = s; > + wmb(); > +} > + > +static inline enum hvutil_device_state > +hvutil_device_get_state(enum hvutil_device_state *p) > +{ > + rmb(); > + return *p; > +} > + > #endif /* _HYPERV_VMBUS_H */ This is crazy. If you need to know the state of something (pun intended) then you had better be using a lock, and not relying on a random pointer to contain a random value and be able to do something based on that. This shows the code is broken, don't paper over things by throwing in random read/write barriers, that is a HUGE flag that something bad is happening here. Just use a lock, that's what it is there for. greg k-h