Re: [PATCH 1/1] Introduce Intel RAPL cooling device driver

[Greg KH]

On Wed, Apr 03, 2013 at 10:35:51AM -0700, Jacob Pan wrote:
> On Wed, 3 Apr 2013 09:35:09 -0700
> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Tue, Apr 02, 2013 at 09:48:18PM -0700, Jacob Pan wrote:
> > > > Let's step back and start over, what exactly are you trying to
> > > > tell userspace?  What data do you have that you need to express
> > > > to it?  How do you want userspace to see/use it?
> > > 
> > > It is a good idea to step back and let me explain what I wanted to
> > > do here for userspace.
> > > 
> > > I have two kinds of applications that might use this driver.
> > > 1. simple use case where user sets a power limit for a RAPL domain.
> > > e.g. set graphics unit power limit to 7w
> > > 2. advanced use case where use can do fine tuning on top of simple
> > > power limit,e.g. the dynamic response parameters of power control
> > > logic, event notifications, etc.
> > > 
> > > For #1, this driver register with the abstract generic thermal layer
> > > (/sys/class/thermal) and presents itself as a set of cooling devices
> > > with a single knob per domain for power limits.
> > > root@chromoly:/sys/class/thermal/cooling_device15# echo 7000 >
> > > cur_state 
> > 
> > Great, how about submitting that functionality as patch 1 of your
> > series?  That seems like a very "normal" thermal driver, right?
> > 
> yes, that would be a normal thermal cooling device driver. I will do
> that first. Thanks for the suggestion.

Do that first, get it merged, and then let's work on the second part.
The patch for that will be much more obvious as to what you are
attempting to do.

> >  Perhaps the thermal interface could be expanded to provide
> > more functionality that you need?
> yes, some of them such as limits. But not all the data in the list
> above are suitable for thermal interface. That is why I am trying to
> balance between abstracted generic data and RAPL specific data while
> still allow linking between the two.

What is not in the existing interface?  And as this is a thermal device,
why can't you add them there?

> The way I envisioned how a thermal/power management app would use is:
> 1. go through generic thermal layer sysfs and find available RAPL
> domains
> 2. if the app wants to do more fine grained control, it follows the
> device symlink to locate the RAPL domain specific sysfs area.

So any application will have to know all of the device-specific
attributes?  That totally defeats the purpose of a generic api that the
kernel is providing.  You are creating device-specific apis that will
not work over the long-run (i.e. next 5-10 years.)  Please don't do that
unless you have exhausted _all_ other alternatives.

So, get your first driver accepted, using the in-kernel thermal api, and
then, if you still feel you wish to do device-specific extensions, we
can discuss that then.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/