Re: [PATCH v2 2/6] coresight: Fail to open with return stacks if they are unavailable

[James Clark <james.clark@arm.com>]

On 28/01/2022 11:24, Suzuki K Poulose wrote:
> Hi James
> 
> On 13/01/2022 09:10, 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.
> 
> Looking at this again (with similar comment to the Branch Broadcast),
> won't it disable using retstack on all CPUs, even when some of them
> support it ?
> 
> i.e., CPU0 - supports retstack, CPU1 - doesn't
> 
> A perf run with retstack will fail, as CPU1 doesn't support it (even
> though we advertise it, unconditionally).
> 
> So, if we ignore the failure, this would still allow CPU0 to use
> the feature and as long as the OpenCSD is able to decode the trace
> we should ignore the failure ?
> 
> I think we may also need to tune the etm4x_enable_hw() to skip
> updating the TRCCONFIGR with features not supported by the ETM
> 

Hi Suzuki,

I'm picking up this branch broadcast change again after the haitus.

For this point, do you think it would be worth distinguishing between "no
known CPUs that support the feature" vs "not currently running on a
CPU that supports it but there are others that do"?

Also would we want to distinguish between per-CPU or per-process events?
For the former it actually is possible to fail to open because all of
the information is known.

I'm just thinking of the case where someone asks for a load of flags
and thinks that they're getting them but get no feedback that they won't.
But I understand having some complicated solution like I'm suggesting
might be even more surprising to users.

Maybe the cleanest solution is to ask users to supply a config that
can work on anywhere the event could possibly be scheduled. It doesn't
really make sense to have retstack on a per-process event on big-little
and then getting half of one type of data and half of another. It would
make more sense to fail to open in that case and they have the choice of
either doing per-CPU events or disabling retstacks altogether.

This seems like a similar problem to the issue causing the Coresight self
test failure where a certain sink was picked that couldn't be reached and
the test failed.

In that case the change we made doesn't quite match up to my suggestion here:

 * Per-cpu but an unreachable sink -> fail
 * Per-process and potentially reachable sink in the future -> pass

Maybe it would have been better to say that the sink always has to be
reachable otherwise is the outcome predicatable?

James

> Suzuki
> 
> 
>>
>> 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 04669ecc0efa..a93c1a5fe045 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);
>> +        }
>> +    }
>>       /*
>>        * Set any selected configuration and preset.
>>        *
>