| Message ID | 20200722172040.1299289-2-suzuki.poulose@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: Support for ETMv4.4 system instructions | expand |
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. > > 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 <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
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;
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(-)