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 ; 10 Oct 2019 21:37:46 -0000 Received: from mga03.intel.com ([134.134.136.65]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iIg80-0003XQ-Mv for speck@linutronix.de; Thu, 10 Oct 2019 23:37:45 +0200 Date: Thu, 10 Oct 2019 14:31:51 -0700 From: Pawan Gupta Subject: [MODERATED] Re: [PATCH v5 09/11] TAAv5 9 Message-ID: <20191010213151.GJ11840@guptapadev.amr> References: <5d983ad2.1c69fb81.e6640.8f51SMTPIN_ADDED_BROKEN@mx.google.com> <20191006170646.GA147859@kroah.com> <20191008060156.GG5154@guptapadev.amr> MIME-Version: 1.0 In-Reply-To: <20191008060156.GG5154@guptapadev.amr> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: 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. Thanks, Pawan