Good day Suzuki,
On Thu, 14 Mar 2019 at 06:17, Suzuki K Poulose suzuki.poulose@arm.com wrote:
Hi Mathieu,
On 06/03/2019 22:57, Mathieu Poirier wrote:
In preparation to handle device reference counting inside of the sink drivers, add a return code to the sink::disable() operation so that proper action can be taken if a sink has not been disabled.
Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org
drivers/hwtracing/coresight/coresight-etb10.c | 3 ++- drivers/hwtracing/coresight/coresight-tmc-etf.c | 5 +++-- drivers/hwtracing/coresight/coresight-tmc-etr.c | 5 +++-- drivers/hwtracing/coresight/coresight-tpiu.c | 3 ++- drivers/hwtracing/coresight/coresight.c | 6 +++++- include/linux/coresight.h | 2 +- 6 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 105782ea64c7..71c2a3cdb866 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -325,7 +325,7 @@ static void etb_disable_hw(struct etb_drvdata *drvdata) coresight_disclaim_device(drvdata->base); }
-static void etb_disable(struct coresight_device *csdev) +static int etb_disable(struct coresight_device *csdev) { struct etb_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); unsigned long flags; @@ -340,6 +340,7 @@ static void etb_disable(struct coresight_device *csdev) spin_unlock_irqrestore(&drvdata->spinlock, flags);
dev_dbg(drvdata->dev, "ETB disabled\n");
return 0;}
static void *etb_alloc_buffer(struct coresight_device *csdev, int cpu,
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index a5f053f2db2c..d4213e7c2c45 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -273,7 +273,7 @@ static int tmc_enable_etf_sink(struct coresight_device *csdev, return 0; }
-static void tmc_disable_etf_sink(struct coresight_device *csdev) +static int tmc_disable_etf_sink(struct coresight_device *csdev) { unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -281,7 +281,7 @@ static void tmc_disable_etf_sink(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags);
return;
return -EBUSY;...
-static void tmc_disable_etr_sink(struct coresight_device *csdev) +static int tmc_disable_etr_sink(struct coresight_device *csdev) { unsigned long flags; struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -1400,7 +1400,7 @@ static void tmc_disable_etr_sink(struct coresight_device *csdev) spin_lock_irqsave(&drvdata->spinlock, flags); if (drvdata->reading) { spin_unlock_irqrestore(&drvdata->spinlock, flags);
return;
return -EBUSY;This patch as such is fine. But I think we can improve the handling here.
I agree that these error codes are appropriate. However, I think we should still modify the mode to disabled for both the etf/etr to make sure the device is kept disabled after we finished reading. As the user has disabled it and thinks that it has completed successfully.
So, I think when we find that the sink buffer is being read, we may :
- Set the mode to disabled, to allow the unprepare routine to keep this
disabled.
- Return 0, to indicate the operation will be acknowledged and no need
to redo the operation.
What do you think ?
We could do that but after looking at the ETR code (I haven't looked at ETF) the following comes to mind:
1) Disabling the ETR when a read is already happening means we call __tmc_etr_disable_hw() twice, that is once in tmc_read_prepare_etr() and another time in tmc_disable_etr_sink() as part of tmc_etr_disable_hw(). I looked at the code and this _should_ be fine. To do things properly we'd need to split tmc_etr_disable_hw() in two, hence creating 3 different levels of ETR disable, which I'm not a fan of.
2) Another problem I see is that by disabling the sink while a read operation is pending, we also set the mode to CS_MODE_DISABLED and decrement csdev->refcnt. That means another session (sysfs or perf) is free to use the sink and do all the required re-initialisation it needs. I think this should be fine but a lot more scrutiny would need to be invested if we are to go down that route. Thanks to you I found a bug in tmc_disable_etf_sink()[1] and tmc_disable_etr_sink()[2] - testing for drvdata->reading should be done before decrementing the refcount.
3) I've always seen sysfs operations being done consecutively, i.e one after the other. If a user is reading from a sink it is not expected they would issue a concurrent disable operation.
So long story short I think it is possible but I'm not sure it it worth the investment. I'm also not sure it should be rolled in this set. Anyways, your turn to let me know what you think.
Mathieu
[1]. https://git.linaro.org/people/mathieu.poirier/coresight.git/tree/drivers/hwt... [2]. https://git.linaro.org/people/mathieu.poirier/coresight.git/tree/drivers/hwt...
Suzuki