On 09/12/2021 11:13, James Clark wrote:
On 09/12/2021 11:00, Suzuki K Poulose wrote:
On 08/12/2021 16:09, James Clark wrote:
Maintain consistency with the other options by failing to open when they aren't supported. For example ETM_OPT_TS, ETM_OPT_CTXTID2 and the newly added ETM_OPT_BRANCH_BROADCAST all return with -EINVAL if they are requested but not supported by hardware.
The consequence of not doing this is that the user may not be aware that they are not enabling the feature as it is silently disabled.
Signed-off-by: James Clark james.clark@arm.com
drivers/hwtracing/coresight/coresight-etm4x-core.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index d2bafb50c66a..0a9bb943a5e5 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -674,10 +674,15 @@ static int etm4_parse_event_config(struct coresight_device *csdev, } /* return stack - enable if selected and supported */ - if ((attr->config & BIT(ETM_OPT_RETSTK)) && drvdata->retstack) - /* bit[12], Return stack enable bit */ - config->cfg |= BIT(12);
+ if (attr->config & BIT(ETM_OPT_RETSTK)) { + if (!drvdata->retstack) { + ret = -EINVAL; + goto out; + } else { + /* bit[12], Return stack enable bit */ + config->cfg |= BIT(12); + }
nit: While at this, please could you change the hard coded value to ETM4_CFG_BIT_RETSTK ?
I started changing them all because I had trouble searching for bits by name but then I thought it would snowball into a bigger change so I undid it.
I think I'll just go and do it now if it's an issue here.
Hi Suzuki,
I started on this and I think the only worthwhile change is to make them all consistent with sysreg.h. As in have xxx_SHIFT and xxx_MASK style definitions like:
#define TRCCONFIGR_INSTP0_SHIFT 1 #define TRCCONFIGR_INSTPO_MASK GENMASK(1,0)
This has been done for SPE and some of the new ETM stuff. If that sounds right to you I will go and do it as a followup patch to this one. It is quite a bit change so I can see maybe we don't want to do it? (Personally I would vote to do it)
James
Otherwise, looks good to me
Suzuki