Re: [PATCH] coresight: cti: Check if the CPU activated for the CPU CTI

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 27/03/2023 15:28, Tao Zhang wrote:
> Hi Suzuki,
> 
> On 3/27/2023 5:52 PM, Suzuki K Poulose wrote:
>> On 27/03/2023 10:49, Tao Zhang wrote:
>>> Check whether the CPU corresponding to the CPU CTI is activated.
>>> If it is not activated, the CPU CTI node should not exist, and
>>> an error will be returned in the initialization function.
>>>
>>> Signed-off-by: Tao Zhang <quic_taozha@quicinc.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-cti-core.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-cti-core.c 
>>> b/drivers/hwtracing/coresight/coresight-cti-core.c
>>> index 277c890..aaa83ae 100644
>>> --- a/drivers/hwtracing/coresight/coresight-cti-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-cti-core.c
>>> @@ -899,10 +899,12 @@ static int cti_probe(struct amba_device *adev, 
>>> const struct amba_id *id)
>>>       drvdata->config.hw_powered = true;
>>>         /* set up device name - will depend if cpu bound or otherwise */
>>> -    if (drvdata->ctidev.cpu >= 0)
>>> +    if (drvdata->ctidev.cpu >= 0) {
>>> +        if (!cpu_active(drvdata->ctidev.cpu))
>>> +            return -ENXIO;
>>>           cti_desc.name = devm_kasprintf(dev, GFP_KERNEL, "cti_cpu%d",
>>>                              drvdata->ctidev.cpu);
>>
>> But why ? As long as we do not enable or touch any CPU specific bits 
>> in the probe, why do we need to fail this ? What are you trying to fix ?
>>
>> Please could you share the log if you are hitting something ? This looks
>> like masking a problem.
>>
>> Suzuki
> 
> We found that when the CPU core is disabled, for example, CPU3 is 
> disabled, but
> 
> CPU3 CTI node corresponding to CPU3 still exists. In fact, in this case, 
> CPU3 CTI
> 
> has been unable to trigger CPU3 properly since CPU3 is in an inactive 
> state. This change
> 
> is to avoid configuring the CPU CTI of the CPU that has been disabled in 
> this case.

Who is configuring the trigger ? Shouldn't we skip "enabling" the CTI
when the associated CPU is inactive instead ? Disabling the probe with
an error doesn't solve the problem. What if the CPU becomes active later 
? What makes sure that the CTI is probed then ?

Suzuki


> 
> Tao
> 
>>
>>
>>> -    else
>>> +    } else
>>>           cti_desc.name = coresight_alloc_device_name(&cti_sys_devs, 
>>> dev);
>>>       if (!cti_desc.name)
>>>           return -ENOMEM;
>>