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

[Suzuki K Poulose]

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@xxxxxxxxxxx>
> > > ---
> > >   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;