Re: [PATCH v3 3/5] coresight: add support for debug module

From	Leo Yan <leo.yan@linaro.org>
Newsgroups	linux.kernel
Subject	Re: [PATCH v3 3/5] coresight: add support for debug module
Date	2017-03-23 08:40 +0100
Message-ID	<to5vJ-7s8-29@gated-at.bofh.it> (permalink)
References	(4 earlier) <tnPhg-3Fu-37@gated-at.bofh.it> <tnQQ3-4OP-59@gated-at.bofh.it> <tnR9o-5eZ-11@gated-at.bofh.it> <tnSf7-5Zo-17@gated-at.bofh.it> <tnSoO-63J-31@gated-at.bofh.it>
Organization	linux.* mail to news gateway

Show all headers | View raw

On Wed, Mar 22, 2017 at 05:25:50PM +0000, Sudeep Holla wrote:
> 
> 
> On 22/03/17 17:09, Suzuki K Poulose wrote:
> > On 22/03/17 16:17, Sudeep Holla wrote:
> 
> [...]
> 
> >> 
> >> Point taken. So we could just specify that all necessary power
> >> domains need to be on for proper functionality for this feature and
> >> that it's highly platform specific instead of mixing cpu/cluster
> >> idle details here.
> >> 
> >>> The key point is that the caveat in using this driver is that
> >>> the power management has to be considered on a platform specific
> >>> basis before it is configured; and appropriate actions may be
> >>> needed for it to work correctly. Without this then the driver
> >>> could cause more issues than it debugs. A user selecting this
> >>> _must_ be told about these issues
> >>> 
> > 
> > So given all the possible caveats, I think we :
> > 
> > 1) Shouldn't enable the driver by default at runtime even if it is 
> > built-in. 
> > 2) Should provide mechanisms to turn it on at boot (via
> > kernel commandline) or anytime later (via sysfs), which kind of puts
> > the responsibility back on the user : "You know what you are doing". 
> > 3) Shouldn't turn the driver on based on "nohlt" which the user
> > could use it for some other purposes, without explicit intention of
> > turning this driver on).
> > 4) Should document the fact that, on some
> > platforms, the user may have to disable CPUidle explicitly to get the
> > driver working. But let us not make it the default. The user with a
> > not so ideal platform could add "nohlt" and get it working.
> > 
> 
> Agreed on all points and well summarized.
> I would like to highlight (3) and (4) as it needs to be well understood.
> 
> "nohlt" has a *different* meaning already, so using that in this
> driver for something else is simple wrong as it affects the system in
> unintended ways. And yes if user (mis)uses it to get things working,
> it's fine but shouldn't be recommended way.

Understand this point.

I will try to use general way to constraint CPUIdle like other
drivers.

Thanks all for these good suggestions :)

Thanks,
Leo Yan

Thread

Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-21 16:50 +0100
  Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 15:20 +0100
    Re: [PATCH v3 3/5] coresight: add support for debug module Mike Leach <mike.leach@linaro.org> - 2017-03-22 17:00 +0100
      Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 17:20 +0100
        Re: [PATCH v3 3/5] coresight: add support for debug module Suzuki K Poulose <Suzuki.Poulose@arm.com> - 2017-03-22 18:30 +0100
          Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 18:40 +0100
            Re: [PATCH v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-23 08:40 +0100
              Re: [PATCH v3 3/5] coresight: add support for debug module Mike Leach <mike.leach@linaro.org> - 2017-03-23 13:30 +0100
    Re: [PATCH v3 3/5] coresight: add support for debug module Leo Yan <leo.yan@linaro.org> - 2017-03-22 17:10 +0100
      Re: [PATCH v3 3/5] coresight: add support for debug module Sudeep Holla <sudeep.holla@arm.com> - 2017-03-22 18:10 +0100

csiph-web