On Fri, Mar 15, 2019 at 04:57:13PM +0000, Suzuki K Poulose wrote:
On 06/03/2019 22:57, Mathieu Poirier wrote:
When operating in CPU-wide mode with an N:1 source/sink HW topology, multiple CPUs can access a sink concurrently. As such reference counting needs to happen when the device's spinlock is held to avoid racing with other operations (start(), update(), stop()).
I think this patch is trying to address the following issue. i.e, we get the refcount a bit late, after we have enabled the sink, and that could make use loose some sessions, if another session drops the refcount before the another one gets it.
i.e,
Session A Session B
enable_sink atomic_inc(refcount) = 1
... atomic_dec(refcount) = 0 enable_sink if (refcount == 0) disable_sink atomic_inc() could we have this in the commit description.
This is only one of the nightmare scenarios that can happen... I'll add the above to the commit log.
Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org
drivers/hwtracing/coresight/coresight-etb10.c | 18 ++++++++++-- .../hwtracing/coresight/coresight-tmc-etf.c | 21 +++++++++++--- .../hwtracing/coresight/coresight-tmc-etr.c | 19 +++++++++++-- drivers/hwtracing/coresight/coresight-tpiu.c | 6 +++- drivers/hwtracing/coresight/coresight.c | 28 +++++++++---------- 5 files changed, 66 insertions(+), 26 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 71c2a3cdb866..dcc550507f70 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -5,6 +5,7 @@
- Description: CoreSight Embedded Trace Buffer driver
*/ +#include <linux/atomic.h> #include <linux/kernel.h> #include <linux/init.h> #include <linux/types.h> @@ -160,12 +161,16 @@ static int etb_enable_sysfs(struct coresight_device *csdev) } /* Nothing to do, the tracer is already enabled. */
- if (drvdata->mode == CS_MODE_SYSFS)
- if (drvdata->mode == CS_MODE_SYSFS) {
atomic_inc(csdev->refcnt);- } ret = etb_enable_hw(drvdata);
- if (!ret)
- if (!ret) { drvdata->mode = CS_MODE_SYSFS;
atomic_inc(csdev->refcnt);- } out:
Could we consolidate the above two cases and move it here at out ?;
We can't consolidate at 'out' because it is also the exit way for the first 'if' condition. What can be done is something like this:
if (drvdata->mode == CS_MODE_PERF) { ret = -EBUSY; goto out; }
if (drvdata->mode == CS_MODE_NONE) { ret = etb_enable_hw(drvdata); if (ret) goto out; }
drvdata->mode = CS_MODE_SYSFS; atomic_inc(csdev->refcnt);
out: spin_unlock_irqrestore(&drvdata->spinlock, flags);
This might be what you had in mind... We just happen to be setting the mode once more if we are already in CS_MODE_SYSFS.
if (!ret) atomic_inc(csdev->refcnt);
spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -196,8 +201,10 @@ static int etb_enable_perf(struct coresight_device *csdev, void *data) goto out; ret = etb_enable_hw(drvdata);
- if (!ret)
- if (!ret) { drvdata->mode = CS_MODE_PERF;
atomic_inc(csdev->refcnt);- } out: spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -332,6 +339,11 @@ static int etb_disable(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags);
- if (atomic_dec_return(csdev->refcnt)) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);return -EBUSY;- }
- /* Disable the ETB only if it needs to */ if (drvdata->mode != CS_MODE_DISABLED) { etb_disable_hw(drvdata);
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index d4213e7c2c45..b6d0c010eec1 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -4,6 +4,7 @@
- Author: Mathieu Poirier mathieu.poirier@linaro.org
*/
..
/* * If drvdata::buf isn't NULL, memory was allocated for a previous @@ -200,11 +203,13 @@ static int tmc_enable_etf_sink_sysfs(struct coresight_device *csdev) } ret = tmc_etb_enable_hw(drvdata);
- if (!ret)
- if (!ret) { drvdata->mode = CS_MODE_SYSFS;
- else
atomic_inc(csdev->refcnt);- } else { /* Free up the buffer if we failed to enable */ used = false;
- } out:
Same as above here.
I'd rather leave this one alone due to the extra complexity around managing the allocated memory.
spin_unlock_irqrestore(&drvdata->spinlock, flags); @@ -239,8 +244,10 @@ static int tmc_enable_etf_sink_perf(struct coresight_device *csdev, void *data) if (ret) break; ret = tmc_etb_enable_hw(drvdata);
if (!ret)
if (!ret) { drvdata->mode = CS_MODE_PERF;atomic_inc(csdev->refcnt);}@@ -279,6 +286,12 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev) struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); spin_lock_irqsave(&drvdata->spinlock, flags);
- if (atomic_dec_return(csdev->refcnt)) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);return -EBUSY;- }
- if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY;
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index 33501777038a..cbb17b2c53b5 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -4,6 +4,7 @@
- Author: Mathieu Poirier mathieu.poirier@linaro.org
*/ +#include <linux/atomic.h> #include <linux/coresight.h> #include <linux/dma-mapping.h> #include <linux/iommu.h> @@ -1124,8 +1125,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) * sink is already enabled no memory is needed and the HW need not be * touched, even if the buffer size has changed. */
- if (drvdata->mode == CS_MODE_SYSFS)
- if (drvdata->mode == CS_MODE_SYSFS) {
atomic_inc(csdev->refcnt);- } /*
- If we don't have a buffer or it doesn't match the requested size,
@@ -1138,8 +1141,10 @@ static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev) } ret = tmc_etr_enable_hw(drvdata, drvdata->sysfs_buf);
- if (!ret)
- if (!ret) { drvdata->mode = CS_MODE_SYSFS;
atomic_inc(csdev->refcnt);- } out: spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -1370,8 +1375,10 @@ static int tmc_enable_etr_sink_perf(struct coresight_device *csdev, void *data) etr_perf->head = PERF_IDX2OFF(handle->head, etr_perf); drvdata->perf_data = etr_perf; rc = tmc_etr_enable_hw(drvdata, etr_perf->etr_buf);
- if (!rc)
- if (!rc) { drvdata->mode = CS_MODE_PERF;
atomic_inc(csdev->refcnt);- } unlock_out: spin_unlock_irqrestore(&drvdata->spinlock, flags);
@@ -1398,6 +1405,12 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev) struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); spin_lock_irqsave(&drvdata->spinlock, flags);
- if (atomic_dec_return(csdev->refcnt)) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);return -EBUSY;- }
- if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags); return -EBUSY;
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c index 0d13da1b9df1..7acbeffcc137 100644 --- a/drivers/hwtracing/coresight/coresight-tpiu.c +++ b/drivers/hwtracing/coresight/coresight-tpiu.c @@ -5,6 +5,7 @@
- Description: CoreSight Trace Port Interface Unit driver
*/ +#include <linux/atomic.h> #include <linux/kernel.h> #include <linux/init.h> #include <linux/device.h> @@ -73,7 +74,7 @@ static int tpiu_enable(struct coresight_device *csdev, u32 mode, void *__unused) struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); tpiu_enable_hw(drvdata);
- atomic_inc(csdev->refcnt); dev_dbg(drvdata->dev, "TPIU enabled\n"); return 0; }
@@ -98,6 +99,9 @@ static int tpiu_disable(struct coresight_device *csdev) { struct tpiu_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
- if (atomic_dec_return(csdev->refcnt))
return -EBUSY;- tpiu_disable_hw(drvdata); dev_dbg(drvdata->dev, "TPIU disabled\n");
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 13eda4693f81..19ba121d7451 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -225,14 +225,13 @@ static int coresight_enable_sink(struct coresight_device *csdev, * We need to make sure the "new" session is compatible with the * existing "mode" of operation. */
- if (sink_ops(csdev)->enable) {
ret = sink_ops(csdev)->enable(csdev, mode, data);if (ret)return ret;csdev->enable = true;- }
- if (!sink_ops(csdev)->enable)
return -EINVAL;
- atomic_inc(csdev->refcnt);
- ret = sink_ops(csdev)->enable(csdev, mode, data);
- if (ret)
return ret;- csdev->enable = true; return 0; }
@@ -241,14 +240,13 @@ static void coresight_disable_sink(struct coresight_device *csdev) { int ret;
- if (atomic_dec_return(csdev->refcnt) == 0) {
if (sink_ops(csdev)->disable) {ret = sink_ops(csdev)->disable(csdev);if (ret)return;csdev->enable = false;}- }
- if (!sink_ops(csdev)->disable)
return;- ret = sink_ops(csdev)->disable(csdev);
- if (ret)
return;- csdev->enable = false; } static int coresight_enable_link(struct coresight_device *csdev,
Otherwise looks fine to me.
Suzuki