Hi Mathieu,
-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:
- 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.
So the point is, if it is being read, we simply set the mode to DISABLED and return success here. That implies, we don't do the __tmc_etr_disable_hw() as the hw is already disabled. Now, the read_unprepare_etr() could leave the ETR disabled, since the mode has been changed already.
- 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.
But then we do check if it is being read before we allow enabling the hardware. So, we are covered. If we don't, we must do that if we are going to enable PERF mode.
- 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.
I am fine with this being delayed into separate patch.
Cheers Suzuki