lkml.org 
[lkml]   [2022]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/3] coresight: Fail to open with return stacks if they are unavailable
    From
    Date


    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

    \
     
     \ /
      Last update: 2022-01-07 16:11    [W:3.135 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site