On 22/04/16 18:14, Mathieu Poirier wrote:
static int tmc_enable_etf_sink(struct coresight_device *csdev, u32 mode)
{
+ bool used = false;
+ char *buf = NULL;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ /* This shouldn't be happening */
+ if (WARN_ON(mode != CS_MODE_SYSFS))
+ return -EINVAL;
+
+ /*
+ * If a buffer is already allocated *keep holding* the lock and
+ * jump to the fast path. Otherwise release the lock and allocate
+ * memory to work with.
+ */
spin_lock_irqsave(&drvdata->spinlock, flags);
+ if (drvdata->buf)
+ goto fast_path;
+
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+ /* Allocating the memory here while outside of the spinlock */
+ buf = kzalloc(drvdata->size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ /* Let's try again */
+ spin_lock_irqsave(&drvdata->spinlock, flags);
+fast_path:
nit: As I mentioned in the previous series, could we not avoid the goto in the
middle of the
function,by doing :
if (!drvdata->buf) {
unlock();
buf = alloc();
if (!buf) return -ENOMEM;
lock();
}
if (drvdata->reading) {
spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ /*
+ * Free allocated memory outside of the spinlock. There is
+ * no need to assert the validity of 'buf' since calling
+ * kfree(NULL) is safe.
+ */
+ kfree(buf);
return -EBUSY;
}
It would be good to unify the exit points as we do similar steps.
if (drvdata->reading) {
rc = -EBUSY;
goto out;
}
+ /*
+ * If drvdata::buf isn't NULL, memory was allocated for a previous
+ * trace run but wasn't read. If so simply zero-out the memory.
+ * Otherwise use the memory allocated above.
+ *
+ * The memory is freed when users read the buffer using the
+ * /dev/xyz.{etf|etb} interface. See tmc_read_unprepare_etf() for
+ * details.
+ */
+ if (drvdata->buf) {
+ memset(drvdata->buf, 0, drvdata->size);
+ } else {
+ used = true;
+ drvdata->buf = buf;
+ }
+
tmc_etb_enable_hw(drvdata);
drvdata->enable = true;
out:
spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ /* Free memory outside the spinlock if need be */
+ if (!used && buf)
+ kfree(buf);
+
if (!rc)
dev_info(drvdata->dev, "TMC-ETB/ETF enabled\n");
return rc
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c
b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 3483d139a4ac..474c70c089f3 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
static int tmc_enable_etr_sink(struct coresight_device *csdev, u32 mode)
{
+ bool used = false;
unsigned long flags;
+ void __iomem *vaddr = NULL;
+ dma_addr_t paddr;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ /* This shouldn't be happening */
+ if (WARN_ON(mode != CS_MODE_SYSFS))
+ return -EINVAL;
+
+ /*
+ * If a buffer is already allocated *keep holding* the lock and
+ * jump to the fast path. Otherwise release the lock and allocate
+ * memory to work with.
+ */
+ spin_lock_irqsave(&drvdata->spinlock, flags);
+ if (drvdata->vaddr)
+ goto fast_path;
+
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+ /*
+ * Contiguous memory can't be allocated while a spinlock is held.
+ * As such allocate memory here and free it if a buffer has already
+ * been allocated (from a previous session).
+ */
+ vaddr = dma_alloc_coherent(drvdata->dev, drvdata->size,
+ &paddr, GFP_KERNEL);
+ if (!vaddr)
+ return -ENOMEM;
+
+ /* Let's try again */
spin_lock_irqsave(&drvdata->spinlock, flags);
+fast_path:
nit: Same as above.
I am not too particular about the above changes, but would be good to have them
reader friendly.
Either way,
Reviewed-by: Suzuki K Poulose <suzuki.poul...@arm.com>
Suzuki
