intel_th: msu: Serialize enabling/disabling
In order to guarantee that readers don't race with trace enabling,
both should happen under the same mutex. Having two mutexes seems
like an overkill, considering that because of the above, they'll
have to be acquired together, around trace enabling and char device
opening.
This patch makes both buffer accesses and readers serialize on
msc::buf_mutex and makes sure that 'enabled' flag accesses are also
serialized on it.
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Laurent Fert <laurent.fert@intel.com>
diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 25af214..ee15306 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -122,7 +122,6 @@
atomic_t mmap_count;
struct mutex buf_mutex;
- struct mutex iter_mutex;
struct list_head iter_list;
/* config */
@@ -257,23 +256,37 @@
iter = kzalloc(sizeof(*iter), GFP_KERNEL);
if (!iter)
- return NULL;
+ return ERR_PTR(-ENOMEM);
+
+ mutex_lock(&msc->buf_mutex);
+
+ /*
+ * Reading and tracing are mutually exclusive; if msc is
+ * enabled, open() will fail; otherwise existing readers
+ * will prevent enabling the msc and the rest of fops don't
+ * need to worry about it.
+ */
+ if (msc->enabled) {
+ kfree(iter);
+ iter = ERR_PTR(-EBUSY);
+ goto unlock;
+ }
msc_iter_init(iter);
iter->msc = msc;
- mutex_lock(&msc->iter_mutex);
list_add_tail(&iter->entry, &msc->iter_list);
- mutex_unlock(&msc->iter_mutex);
+unlock:
+ mutex_unlock(&msc->buf_mutex);
return iter;
}
static void msc_iter_remove(struct msc_iter *iter, struct msc *msc)
{
- mutex_lock(&msc->iter_mutex);
+ mutex_lock(&msc->buf_mutex);
list_del(&iter->entry);
- mutex_unlock(&msc->iter_mutex);
+ mutex_unlock(&msc->buf_mutex);
kfree(iter);
}
@@ -454,7 +467,6 @@
{
struct msc_window *win;
- mutex_lock(&msc->buf_mutex);
list_for_each_entry(win, &msc->win_list, entry) {
unsigned int blk;
size_t hw_sz = sizeof(struct msc_block_desc) -
@@ -466,7 +478,6 @@
memset(&bdesc->hw_tag, 0, hw_sz);
}
}
- mutex_unlock(&msc->buf_mutex);
}
/**
@@ -474,12 +485,15 @@
* @msc: the MSC device to configure
*
* Program storage mode, wrapping, burst length and trace buffer address
- * into a given MSC. If msc::enabled is set, enable the trace, too.
+ * into a given MSC. Then, enable tracing and set msc::enabled.
+ * The latter is serialized on msc::buf_mutex, so make sure to hold it.
*/
static int msc_configure(struct msc *msc)
{
u32 reg;
+ lockdep_assert_held(&msc->buf_mutex);
+
if (msc->mode > MSC_MODE_MULTI)
return -ENOTSUPP;
@@ -497,21 +511,19 @@
reg = ioread32(msc->reg_base + REG_MSU_MSC0CTL);
reg &= ~(MSC_MODE | MSC_WRAPEN | MSC_EN | MSC_RD_HDR_OVRD);
+ reg |= MSC_EN;
reg |= msc->mode << __ffs(MSC_MODE);
reg |= msc->burst_len << __ffs(MSC_LEN);
- /*if (msc->mode == MSC_MODE_MULTI)
- reg |= MSC_RD_HDR_OVRD; */
+
if (msc->wrap)
reg |= MSC_WRAPEN;
- if (msc->enabled)
- reg |= MSC_EN;
iowrite32(reg, msc->reg_base + REG_MSU_MSC0CTL);
- if (msc->enabled) {
- msc->thdev->output.multiblock = msc->mode == MSC_MODE_MULTI;
- intel_th_trace_enable(msc->thdev);
- }
+ msc->thdev->output.multiblock = msc->mode == MSC_MODE_MULTI;
+ intel_th_trace_enable(msc->thdev);
+ msc->enabled = 1;
+
return 0;
}
@@ -521,15 +533,14 @@
* @msc: MSC device to disable
*
* If @msc is enabled, disable tracing on the switch and then disable MSC
- * storage.
+ * storage. Caller must hold msc::buf_mutex.
*/
static void msc_disable(struct msc *msc)
{
unsigned long count;
u32 reg;
- if (!msc->enabled)
- return;
+ lockdep_assert_held(&msc->buf_mutex);
intel_th_trace_disable(msc->thdev);
@@ -569,33 +580,35 @@
static int intel_th_msc_activate(struct intel_th_device *thdev)
{
struct msc *msc = dev_get_drvdata(&thdev->dev);
- int ret = 0;
+ int ret = -EBUSY;
if (!atomic_inc_unless_negative(&msc->user_count))
return -ENODEV;
- mutex_lock(&msc->iter_mutex);
- if (!list_empty(&msc->iter_list))
- ret = -EBUSY;
- mutex_unlock(&msc->iter_mutex);
+ mutex_lock(&msc->buf_mutex);
- if (ret) {
+ /* if there are readers, refuse */
+ if (list_empty(&msc->iter_list))
+ ret = msc_configure(msc);
+
+ mutex_unlock(&msc->buf_mutex);
+
+ if (ret)
atomic_dec(&msc->user_count);
- return ret;
- }
- msc->enabled = 1;
-
- return msc_configure(msc);
+ return ret;
}
static void intel_th_msc_deactivate(struct intel_th_device *thdev)
{
struct msc *msc = dev_get_drvdata(&thdev->dev);
- msc_disable(msc);
-
- atomic_dec(&msc->user_count);
+ mutex_lock(&msc->buf_mutex);
+ if (msc->enabled) {
+ msc_disable(msc);
+ atomic_dec(&msc->user_count);
+ }
+ mutex_unlock(&msc->buf_mutex);
}
/**
@@ -1035,8 +1048,8 @@
return -EPERM;
iter = msc_iter_install(msc);
- if (!iter)
- return -ENOMEM;
+ if (IS_ERR(iter))
+ return PTR_ERR(iter);
file->private_data = iter;
@@ -1101,11 +1114,6 @@
if (!atomic_inc_unless_negative(&msc->user_count))
return 0;
- if (msc->enabled) {
- ret = -EBUSY;
- goto put_count;
- }
-
if (msc->mode == MSC_MODE_SINGLE && !msc->single_wrap)
size = msc->single_sz;
else
@@ -1254,8 +1262,6 @@
msc->mode = MSC_MODE_MULTI;
mutex_init(&msc->buf_mutex);
INIT_LIST_HEAD(&msc->win_list);
-
- mutex_init(&msc->iter_mutex);
INIT_LIST_HEAD(&msc->iter_list);
msc->burst_len =