Messages in this thread | | | Subject | Re: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable | From | James Clark <> | Date | Fri, 7 Jan 2022 15:10:45 +0000 |
| |
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
| |