Skip cpu save/restore before the coresight device is registered.
Cc: Mathieu Poirier <[email protected]>
Cc: Mike Leach <[email protected]>
Signed-off-by: Suzuki K Poulose <[email protected]>
---
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;
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;
+
if (!drvdata->save_state)
return NOTIFY_OK;
--
2.24.1
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 <[email protected]>
> Cc: Mike Leach <[email protected]>
> Signed-off-by: Suzuki K Poulose <[email protected]>
> ---
> 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.
>
> 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.
> +
> if (!drvdata->save_state)
> return NOTIFY_OK;
>
> --
> 2.24.1
>
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 <[email protected]>
>> Cc: Mike Leach <[email protected]>
>> Signed-off-by: Suzuki K Poulose <[email protected]>
>> ---
>> 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