linux-kernel - Re: [v3 3/5] coresight: add support for debug module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Mar 2017 14:41:59 -0600
From:   Mathieu Poirier <mathieu.poirier@...aro.org>
To:     Suzuki K Poulose <Suzuki.Poulose@....com>
Cc:     Leo Yan <leo.yan@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Wei Xu <xuwei5@...ilicon.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        John Stultz <john.stultz@...aro.org>,
        Guodong Xu <guodong.xu@...aro.org>,
        Haojian Zhuang <haojian.zhuang@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>, linux-clk@...r.kernel.org,
        Mike Leach <mike.leach@...aro.org>,
        Sudeep Holla <Sudeep.Holla@....com>
Subject: Re: [v3 3/5] coresight: add support for debug module

On 15 March 2017 at 10:44, Suzuki K Poulose <Suzuki.Poulose@....com> wrote:
> On 13/03/17 16:56, Mathieu Poirier wrote:
>>
>> On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote:
>>>>>>
>>>>>> +
>>>>>> +       put_online_cpus();
>>>>>> +
>>>>>> +       if (!debug_count++)
>>>>>> +               atomic_notifier_chain_register(&panic_notifier_list,
>>>>>> +                                              &debug_notifier);
>>>>>> +
>>>>>
>>>>>
>>>>>> +       sprintf(buf, (char *)id->data, drvdata->cpu);
>>>>>> +       dev_info(dev, "%s initialized\n", buf);
>>>>>
>>>>>
>>>>> This could simply be :
>>>>>         dev_info(dev, "Coresight debug-CPU%d initialized\n",
>>>>> drvdata->cpu);
>>>>>
>>>>> and get rid of the static string and the buffer, see below.
>>>
>>>
>>> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from
>>> AMBA
>>> device probe.
>>
>>
>> Good point.
>>
>>> More on that below.
>>>
>>>>>
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static struct amba_id debug_ids[] = {
>>>>>> +       {       /* Debug for Cortex-A53 */
>>>>>> +               .id     = 0x000bbd03,
>>>>>> +               .mask   = 0x000fffff,
>>>>>
>>>>>
>>>>> ...
>>>>>
>>>>>> +               .data   = "Coresight debug-CPU%d",
>>>>>
>>>>>
>>>>> I think this is pointless, as the debug area we are interested in is
>>>>> always associated
>>>>> with a CPU, we could as well figure out what to print from the
>>>>> drvdata->cpu above.
>>>>
>>>>
>>>> I prefer to follow your suggestion for upper two comments; but I'd like
>>>> check with Mathieu, due I followed up Mathieu's suggestion to write
>>>> current code.
>>>
>>>
>>> Btw, I don't see any PM calls to make sure the power domain (at least the
>>> debug domain)
>>> is up, which could cause problems with accesses to some of these
>>> registers (leave alone the
>>> ones in CPU power domain), especially the EDPRSR. We could also do
>>> pm_runtime_get on the
>>> CPU's power domain, if the CPU is online, before we access the pcsr.
>>
>>
>> I thought about PM runtime operations a little while back but wondered if
>> it is
>> really a good thing to have them around.  When this code is called the
>> system
>> has crashed and as such making PM runtimes call isn't a good idea.
>
>
> You are right. It is not safe to make such calls when we have crashed.
> The other side effect is, if we don't have the debug power domain up,
> we could possibly hang the system and prevent other registered notifiers
> from running, which doesn't sound good either.
>
>>
>> One thing we could do is _not_ call pm_runtime_put() at the end of the
>> probe()
>> operation.  That way we wouldn't have to mess around with PM runtime
>> operations
>> on an unstable system.  This, of course, is costly in terms of power
>> consumption
>> but the system is under test/debug anyway.
>
>
> May be control the behavior via kernel command line ? Something like
> coresight_debug={on or 1} or
> even use the "nohlt" ?

We need to deal with the debug and CPU power domains.

For the former I suggest we do what coresight does and use the
"power-domains" binding[1].  For the CPU power domain we can re-use
the "nohlt" flag.  In the probe function if the "nohlt" cmd line flag
is not set the code bails out.  If it is set pm_runtime_put() is _not_
called and the driver can be used without worries of hanging the
system when the panic handler is invoked.

Am I forgetting something?

[1]. http://lxr.free-electrons.com/source/arch/arm64/boot/dts/arm/juno-base.dtsi#L137

>
> Suzuki