Re: [RFC PATCH 01/14] coresight: etm4x: Skip save/restore before device registration

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

On 07/29/2020 07:01 PM, Mathieu Poirier wrote:
> Hi Suzuki,
> 
> I have starte to review this - comments will be scattered over a few days.
> 
> On Wed, Jul 22, 2020 at 06:20:27PM +0100, Suzuki K Poulose wrote:
>> Skip cpu save/restore before the coresight device is registered.
>>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: Mike Leach <mike.leach@linaro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-etm4x.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
>> index 6d7d2169bfb2..cb83fb77ded6 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
>> @@ -1135,7 +1135,13 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>>   {
>>   	int i, ret = 0;
>>   	struct etmv4_save_state *state;
>> -	struct device *etm_dev = &drvdata->csdev->dev;
>> +	struct coresight_device *csdev = drvdata->csdev;
>> +	struct device *etm_dev;
>> +
>> +	if (WARN_ON(!csdev))
>> +		return -ENODEV;
>> +
>> +	etm_dev = &csdev->dev;
>>   
>>   	/*
>>   	 * As recommended by 3.4.1 ("The procedure when powering down the PE")
>> @@ -1261,6 +1267,10 @@ static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>>   {
>>   	int i;
>>   	struct etmv4_save_state *state = drvdata->save_state;
>> +	struct coresight_device *csdev = drvdata->csdev;
>> +
>> +	if (WARN_ON(!csdev))
>> +		return;
> 
> Restore and save operations are only called from etm4_cpu_pm_notify() where the
> check for a valid drvdata->csdev is already done.
> 

Correct, this is just an enforcement as we are going to rely on the 
availability of drvdata->csdev to access the device with the
introduction of abstraction. This is why we WARN_ON() as we should
never hit this case.

>>   
>>   	CS_UNLOCK(drvdata->base);
>>   
>> @@ -1368,6 +1378,10 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd,
>>   
>>   	drvdata = etmdrvdata[cpu];
>>   
>> +	/* If we have not registered the device there is nothing to do */
>> +	if (!drvdata->csdev)
>> +		return NOTIFY_OK;
> 
> Can you describe the scenario you've seen this happening in?  Probably best to
> add it to the changelog.

The CPU PM notifier is registered with the probing of the first ETM
device. Now, another ETM device could be probed (on a different CPU
than the parent of this ETM). Now, there is a very narrow window of
time between :

1) Initialise etmdrvdata[cpu]

2) Register the coresight_device for the ETM.(i.e, coresight_register()).

If the CPU is put on idle, after (1) and before (2), we end up with

drvdata->csdev == NULL.

This is unacceptable and there is no need to take an action in
such case. This patch fixes the potential problem, also making
sure that we have the access methods available when we need it.
(drvdata->csdev->access)

I will add it to the commit message.

Cheers
Suzuki