lkml.org 
[lkml]   [2020]   [Jul]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH 01/14] coresight: etm4x: Skip save/restore before device registration
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-07-30 16:42    [W:6.484 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site