From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (193.142.43.55:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 11 Oct 2019 08:45:59 -0000 Received: from mail.kernel.org ([198.145.29.99]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iIqYg-0004Z9-B4 for speck@linutronix.de; Fri, 11 Oct 2019 10:45:59 +0200 Date: Fri, 11 Oct 2019 10:45:48 +0200 From: Greg KH Subject: [MODERATED] Re: [PATCH v5 09/11] TAAv5 9 Message-ID: <20191011084548.GA1075470@kroah.com> References: <5d983ad2.1c69fb81.e6640.8f51SMTPIN_ADDED_BROKEN@mx.google.com> <20191006170646.GA147859@kroah.com> <20191008060156.GG5154@guptapadev.amr> <20191010213151.GJ11840@guptapadev.amr> MIME-Version: 1.0 In-Reply-To: <20191010213151.GJ11840@guptapadev.amr> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Thu, Oct 10, 2019 at 02:31:51PM -0700, speck for Pawan Gupta wrote: > On Mon, Oct 07, 2019 at 11:01:56PM -0700, speck for Pawan Gupta wrote: > > > > +static DEFINE_MUTEX(tsx_mutex); > > > > > > I think I asked this before, but in looking at the code I still can't > > > figure it out. What exactly is this protecting? > > > > > > It looks like you want to keep only one "writer" out of the sysfs store > > > function at a time, but: > > > > > > > +ssize_t hw_tx_mem_store(struct device *dev, struct device_attribute *attr, > > > > + const char *buf, size_t count) > > > > +{ > > > > + enum tsx_ctrl_states requested_state; > > > > + ssize_t ret; > > > > + bool val; > > > > + > > > > + ret = kstrtobool(buf, &val); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + mutex_lock(&tsx_mutex); > > > > + > > > > + if (val) { > > > > + tsx_user_cmd = TSX_USER_CMD_ON; > > > > + requested_state = TSX_CTRL_ENABLE; > > > > + } else { > > > > + tsx_user_cmd = TSX_USER_CMD_OFF; > > > > + requested_state = TSX_CTRL_DISABLE; > > > > + } > > > > + > > > > + /* Current state is same as the reqested state, do nothing */ > > > > + if (tsx_ctrl_state == requested_state) > > > > + goto exit; > > > > + > > > > + tsx_ctrl_state = requested_state; > > > > + > > > > + tsx_update_on_each_cpu(val); > > > > +exit: > > > > + mutex_unlock(&tsx_mutex); > > > > > > What I think you want to do is just protect the tsx_update_on_each_cpu() > > > function, right? > > > > Also I believe below two operations needs to be under a lock. Without > > the lock if there are two writers and one is preempted in between these > > operations there is a possibility that tsx_ctrl_state and TSX hardware > > state could go out of sync. > > > > tsx_ctrl_state = requested_state; > > > > // 1st writer gets preempted here > > // 2nd writer flips tsx_ctrl_state and writes to the MSR. > > // 1st writer wakes up and only writes to the MSR > > > > tsx_update_on_each_cpu(val); > > // tsx_ctrl_state and hardware state would be different here. > > > > Chances of this happening is rare but still a possibility. The lock > > would prevent such a condition. > > Greg, is it not a possible scenario for tsx_ctrl_state and MSR write to > be under the lock. I'm sorry, but I do not understand what you are trying to state here. Look at the code, and the lock, and determine what it really is doing and document that please. Right now I can't determine it and it looks pointless to me, so I must be missing something. thanks, greg k-h