hwmon API update

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

We can already change the clocks of the card, but we need a way to monitor the temperature and bump the fan speed if needed. Another problem with letting users mess with the i2c driver by themselves is that some cards use the i2c driver for fan management while others don't. This is why I would like to introduce nouveau as an hwmon driver, exporting the temperature, fan management and clock speeds so as we can use the thermal zone to monitor the temperature and react when needed.

So far, we use:
- w83l785ts
- w83781d
- adt7473 (most common one)
- f75375
- lm99

With the help of Matthew Garret, I updated his previous proposal for an in-kernel API for hwmon. The patch should apply cleanly on Linux 2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Looking forward to your review and feedback.

Martin

From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001
From: Martin Peres <[email protected]>
Date: Sun, 13 Feb 2011 11:35:17 +0100
Subject: [PATCH] hwmon API update

Original creator: Matthew Garrett <[email protected]>

Signed-off-by Martin Peres <[email protected]>

Quoted 35 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

We can already change the clocks of the card, but we need a way to monitor the temperature and bump the fan speed if needed. Another problem with letting users mess with the i2c driver by themselves is that some cards use the i2c driver for fan management while others don't. This is why I would like to introduce nouveau as an hwmon driver, exporting the temperature, fan management and clock speeds so as we can use the thermal zone to monitor the temperature and react when needed.

So far, we use:
- w83l785ts
- w83781d
- adt7473 (most common one)
- f75375
- lm99

With the help of Matthew Garret, I updated his previous proposal for an in-kernel API for hwmon. The patch should apply cleanly on Linux 2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Looking forward to your review and feedback.

Martin
From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001
From: Martin Peres<[email protected]>
Date: Sun, 13 Feb 2011 11:35:17 +0100
Subject: [PATCH] hwmon API update

Quoted 3 lines

Original creator: Matthew Garrett<[email protected]>

Signed-off-by Martin Peres<[email protected]>

This is an extremely complex change just for the benefit of one driver, with a huge potential of misuse. The changes required in each driver to actually implement the API are substantial, and pretty much only add complexity to each hwmon driver with no real benefit.

The cost gets even larger if one has to consider that some may want or have to to backport drivers to earlier kernel versions. This patchset would result in significant efforts to do such backports.

For the API itself, there are lots of functions with similar parameters, and those parameters are needed in the drivers to determine which attribute is affected. A single function would have accomplished the same, as the drivers will need case statements anyway to identify the actual attribute to be read or written. What we end up here with is a large number of functions to be supported by each driver, all with pretty much the same set of arguments.

I don't know what current thinking is about kernel size increases, but it looks like this patch will result in quite significant kernel size increase (some 18*8 = 144 bytes per driver for all the pointers, plus the actual functions, adds up to a lot). Again this would be with no benefit for most of the users of the hwmon subsystem. Sure, one can argue that the size increases will only occur if the drivers are actually loaded, but that is a pretty weak argument since the code size increase will still show up in each driver.

In summary I am not in favor for this change. Maybe Jean thinks differently, but for my part I don't plan to approve it.

Guenter

Quoted 5 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Quoted 1 line

We can already change the clocks of the card, but we need a way to

Quoted 1 line

monitor the temperature and bump the fan speed if needed.

Quoted 3 lines

Another problem with letting users mess with the i2c driver by themselves is that some cards use the i2c driver for fan management while others don't. This is why I would like to introduce nouveau as an

Quoted 3 lines

hwmon driver, exporting the temperature, fan management and clock speeds so as we can use the thermal zone to monitor the temperature and react when needed.

Quoted 9 lines

So far, we use:
- w83l785ts
- w83781d
- adt7473 (most common one)
- f75375
- lm99

With the help of Matthew Garret, I updated his previous proposal for an in-kernel API for hwmon. The patch should apply cleanly on Linux

Quoted 2 lines

2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Quoted 1 line

Looking forward to your review and feedback.

Quoted 5 lines

From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001
From: Martin Peres <[email protected]>
Date: Sun, 13 Feb 2011 11:35:17 +0100
Subject: [PATCH] hwmon API update

Quoted 3 lines

Original creator: Matthew Garrett <[email protected]>

Signed-off-by Martin Peres <[email protected]>

This is an extremely complex change just for the benefit of one driver, with a huge potential of misuse. The changes required in each driver to actually implement the API are substantial, and pretty much only add complexity to each hwmon driver with no real benefit.

The cost gets even larger if one has to consider that some may want or have to to backport drivers to earlier kernel versions. This patchset would result in significant efforts to do such backports.

For the API itself, there are lots of functions with similar parameters, and those parameters are needed in the drivers to determine which attribute is affected. A single function would have accomplished the same, as the drivers will need case statements anyway to identify the actual attribute to be read or written. What we end up here with is a large number of functions to be supported by each driver, all with pretty much the same set of arguments.

I don't know what current thinking is about kernel size increases, but it looks like this patch will result in quite significant kernel size increase (some 18*8 = 144 bytes per driver for all the pointers, plus the actual functions, adds up to a lot). Again this would be with no benefit for most of the users of the hwmon subsystem. Sure, one can argue that the size increases will only occur if the drivers are actually loaded, but that is a pretty weak argument since the code size increase will still show up in each driver.

In summary I am not in favor for this change. Maybe Jean thinks differently, but for my part I don't plan to approve it.

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

though I'm not following this as closely as I should so I may have missed something.

Dave.

Quoted 7 lines · Jean Delvare

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

Quoted 24 lines · Dave Airlie

Quoted 7 lines · Jean Delvare

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Quoted 7 lines

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

This is not a matter of "no interest". Interest is there, but if one demands too much one may get nothing.

Request for b) so far was "no userspace access", period. This is unacceptable since providing userspace access to monitored values is the whole point of hwmon.

I could imagine an API that covers both a) and b), as long as b) focuses on the "sanitize" aspect and doesn't try to limit userspace access to attributes.

Guenter

Le 03/03/2011 16:22, Guenter Roeck a écrit :

Quoted 37 lines · Dave Airlie

Quoted 24 lines · Jean Delvare

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Quoted 7 lines

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

This is not a matter of "no interest". Interest is there, but if one demands too much one may get nothing.

Request for b) so far was "no userspace access", period. This is unacceptable since providing userspace access to monitored values is the whole point of hwmon.

Quoted 5 lines

I could imagine an API that covers both a) and b), as long as b) focuses on the "sanitize" aspect and doesn't try to limit userspace access to attributes.

Guenter

b) was introduced by Dave, I never asked for it because I don't mind duplicating sensor data (one hwmon device named nouveau and one for the raw access to the i2c chip).

My only wish was to provide a simple way for users to read/change their fan speed and get the GPU temperature no matter if their card have an i2c controller or not.
I do agree that sanitizing could be of interest, I especially think about tweaking the temperature value with parameters stored inside the vbios.

I am fully open to suggestions as long as it involves sharing the code in one way or another.

Martin
_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

Am Donnerstag, den 03.03.2011, 18:29 +0100 schrieb Martin Peres:

Quoted 38 lines

Le 03/03/2011 16:22, Guenter Roeck a écrit :

Quoted 37 lines · Dave Airlie

Quoted 24 lines · Jean Delvare

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Quoted 7 lines

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

This is not a matter of "no interest". Interest is there, but if one demands too much one may get nothing.

Request for b) so far was "no userspace access", period. This is unacceptable since providing userspace access to monitored values is the whole point of hwmon.

And that is what we want to do. But it would be nice if the graphics drivers could provide a _single_ interface to userspace. Not all boards have i2c hardware monitoring chips and with a single interface we could fall back to the internal gpu sensor, transparently for the user.

Quoted 8 lines

Quoted 5 lines

I could imagine an API that covers both a) and b), as long as b) focuses on the "sanitize" aspect and doesn't try to limit userspace access to attributes.

Guenter

b) was introduced by Dave, I never asked for it because I don't mind duplicating sensor data (one hwmon device named nouveau and one for the raw access to the i2c chip).

Sorry for the confusion Martin, I brought up the point of limiting userspace access and did not cc the nouveau mailing list. I think it is bad behaviour to expose values to userspace which are totally off the real values. This confuses users and should be avoided, especially since we can provide sanitized values.

Why should we push the logic and api for sanitizing the values to many hwmon drivers if we could easily do this at a single point in the graphics driver, if we provide the userspace interface ourself and use the hwmon driver only to instrument the monitoring chip?

Quoted 60 lines

Am Donnerstag, den 03.03.2011, 18:29 +0100 schrieb Martin Peres:

Quoted 38 lines

Le 03/03/2011 16:22, Guenter Roeck a écrit :

Quoted 37 lines · Dave Airlie

Quoted 24 lines · Jean Delvare

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Quoted 7 lines

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

This is not a matter of "no interest". Interest is there, but if one demands too much one may get nothing.

Request for b) so far was "no userspace access", period. This is unacceptable since providing userspace access to monitored values is the whole point of hwmon.

And that is what we want to do. But it would be nice if the graphics drivers could provide a _single_ interface to userspace. Not all boards have i2c hardware monitoring chips and with a single interface we could fall back to the internal gpu sensor, transparently for the user.

Quoted 8 lines

Quoted 5 lines

I could imagine an API that covers both a) and b), as long as b) focuses on the "sanitize" aspect and doesn't try to limit userspace access to attributes.

Guenter

b) was introduced by Dave, I never asked for it because I don't mind duplicating sensor data (one hwmon device named nouveau and one for the raw access to the i2c chip).

Sorry for the confusion Martin, I brought up the point of limiting userspace access and did not cc the nouveau mailing list. I think it is bad behaviour to expose values to userspace which are totally off the real values. This confuses users and should be avoided, especially since we can provide sanitized values.

Quoted 4 lines

Why should we push the logic and api for sanitizing the values to many hwmon drivers if we could easily do this at a single point in the graphics driver, if we provide the userspace interface ourself and use the hwmon driver only to instrument the monitoring chip?

I don't think the functionality (nor the internal API) should have to be implemented in individual drivers. Instead, there should be a new API between hwmon drivers and the hwmon core. Using this API, the hwmon core would read/write raw values from/to hwmon drivers and make those values available to userland and to other drivers (such as the nouveau driver). The hwmon core would then also perform sanitizing if necessary, ie call registered sanitizing functions.

With this approach, hwmon would report sanitized values, graphics drivers would not have to support/generate hwmon sysfs ABI attributes, and hwmon driver structure would be much simpler, since drivers could concentrate on getting data from and to chips instead of having to deal with sysfs attributes. That would be a win for everyone.

Guenter

Am Donnerstag, den 03.03.2011, 13:19 -0800 schrieb Guenter Roeck:

Quoted 80 lines · Lucas Stach

Quoted 60 lines

Am Donnerstag, den 03.03.2011, 18:29 +0100 schrieb Martin Peres:

Quoted 38 lines

Le 03/03/2011 16:22, Guenter Roeck a écrit :

Quoted 37 lines · Dave Airlie

Quoted 24 lines · Jean Delvare

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Quoted 7 lines

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

This is not a matter of "no interest". Interest is there, but if one demands too much one may get nothing.

Request for b) so far was "no userspace access", period. This is unacceptable since providing userspace access to monitored values is the whole point of hwmon.

And that is what we want to do. But it would be nice if the graphics drivers could provide a _single_ interface to userspace. Not all boards have i2c hardware monitoring chips and with a single interface we could fall back to the internal gpu sensor, transparently for the user.

Quoted 8 lines

Quoted 5 lines

I could imagine an API that covers both a) and b), as long as b) focuses on the "sanitize" aspect and doesn't try to limit userspace access to attributes.

Guenter

b) was introduced by Dave, I never asked for it because I don't mind duplicating sensor data (one hwmon device named nouveau and one for the raw access to the i2c chip).

Sorry for the confusion Martin, I brought up the point of limiting userspace access and did not cc the nouveau mailing list. I think it is bad behaviour to expose values to userspace which are totally off the real values. This confuses users and should be avoided, especially since we can provide sanitized values.

Quoted 4 lines

Why should we push the logic and api for sanitizing the values to many hwmon drivers if we could easily do this at a single point in the graphics driver, if we provide the userspace interface ourself and use the hwmon driver only to instrument the monitoring chip?

I don't think the functionality (nor the internal API) should have to be implemented in individual drivers. Instead, there should be a new API between hwmon drivers and the hwmon core. Using this API, the hwmon core would read/write raw values from/to hwmon drivers and make those values available to userland and to other drivers (such as the nouveau driver). The hwmon core would then also perform sanitizing if necessary, ie call registered sanitizing functions.

With this approach, hwmon would report sanitized values, graphics drivers would not have to support/generate hwmon sysfs ABI attributes, and hwmon driver structure would be much simpler, since drivers could concentrate on getting data from and to chips instead of having to deal with sysfs attributes. That would be a win for everyone.

Quoted 1 line

Guenter

This is a bigger change than we initially aimed for and I didn't dare to ask for such a heavy modification, but I'm very happy with this solution if you prefer and support it this way.

u have no objections I will try to hack something together by this weekend so we could further discuss the issue with some real code at hand.

Quoted 77 lines · Lucas Stach

Am Donnerstag, den 03.03.2011, 13:19 -0800 schrieb Guenter Roeck:

Quoted 76 lines · Lucas Stach

Quoted 57 lines

Am Donnerstag, den 03.03.2011, 18:29 +0100 schrieb Martin Peres:

Quoted 30 lines · Guenter Roeck

Quoted 29 lines · Dave Airlie

Quoted 28 lines · Jean Delvare

Quoted 7 lines · Guenter Roeck

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Martin,

you probably should have cc'ed Matthew since it was his patch you based this on, and I think he can provide a good explaination.

to clarify some points,

radeon does probably want something exactly like this, we just haven't gotten to it completely yet, I'd rather not have two drivers in the kernel for exact same hardware,
and I believe sharing the hwmon code to do what we want is a good plan since you don't go around reinventing wheels, but if hwmon/i2c maintainers have no interest
it leaves with little choice but to implement about 5-10 i2c drivers again in drm codebase.

Maybe hwmon/i2c maintainers could suggest a cleaner way to implement what we want,
which I think I can summarize as

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

a) access to monitored values in-kernel
b) no userspace access to the same values except via sanitised via the driver.

This is not a matter of "no interest". Interest is there, but if one demands too much one may get nothing.

Request for b) so far was "no userspace access", period. This is unacceptable since providing userspace access to monitored values is the whole point of hwmon.

And that is what we want to do. But it would be nice if the graphics drivers could provide a _single_ interface to userspace. Not all boards have i2c hardware monitoring chips and with a single interface we could fall back to the internal gpu sensor, transparently for the user.

Quoted 7 lines

Quoted 4 lines

I could imagine an API that covers both a) and b), as long as b) focuses on the "sanitize" aspect and doesn't try to limit userspace access to attributes.

Guenter

b) was introduced by Dave, I never asked for it because I don't mind duplicating sensor data (one hwmon device named nouveau and one for the raw access to the i2c chip).

Sorry for the confusion Martin, I brought up the point of limiting userspace access and did not cc the nouveau mailing list. I think it is bad behaviour to expose values to userspace which are totally off the real values. This confuses users and should be avoided, especially since we can provide sanitized values.

Quoted 4 lines

Why should we push the logic and api for sanitizing the values to many hwmon drivers if we could easily do this at a single point in the graphics driver, if we provide the userspace interface ourself and use the hwmon driver only to instrument the monitoring chip?

I don't think the functionality (nor the internal API) should have to be implemented in individual drivers. Instead, there should be a new API between hwmon drivers and the hwmon core. Using this API, the hwmon core would read/write raw values from/to hwmon drivers and make those values available to userland and to other drivers (such as the nouveau driver). The hwmon core would then also perform sanitizing if necessary, ie call registered sanitizing functions.

With this approach, hwmon would report sanitized values, graphics drivers would not have to support/generate hwmon sysfs ABI attributes, and hwmon driver structure would be much simpler, since drivers could concentrate on getting data from and to chips instead of having to deal with sysfs attributes. That would be a win for everyone.

Quoted 4 lines

Quoted 1 line

Guenter

This is a bigger change than we initially aimed for and I didn't dare to ask for such a heavy modification, but I'm very happy with this solution if you prefer and support it this way.

If done right, it should be much less invasive than the previous approach - meaning existing drivers would not have to be modified and can be converted as time permits.

Quoted 4 lines

u have no objections I will try to hack something together by this weekend so we could further discuss the issue with some real code at hand.

Sure, go ahead. I started something too, but I don't really have much time right now.

Guenter

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Guenter

This is a bigger change than we initially aimed for and I didn't dare to ask for such a heavy modification, but I'm very happy with this solution if you prefer and support it this way.

If done right, it should be much less invasive than the previous approach - meaning existing drivers would not have to be modified and can be converted as time permits.

The previous solution already permitted that, but anyway, it's good to see you welcome change.

On Thu, Mar 03, 2011 at 06:53:12PM -0500, Martin Peres wrote:
[ ... ]

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Guenter

This is a bigger change than we initially aimed for and I didn't dare to ask for such a heavy modification, but I'm very happy with this solution if you prefer and support it this way.

If done right, it should be much less invasive than the previous approach - meaning existing drivers would not have to be modified and can be converted as time permits.

The previous solution already permitted that, but anyway, it's good to see you welcome change.

It touched all drivers. It did not move anything out of drivers, forcing drivers supporting the new functions to support both sysfs _and_ the new functions. Significant difference.

The idea here - at least mine - is to move sysfs attribute management into core hwmon code.

Guenter

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Why? You already have a way to get data out of and send commands to the I2C devices themselves (Using the i2c_smbus_* functions). Why do you insist on going through the I2C device drivers?

Quoted 1 line

Quoted 1 line

We can already change the clocks of the card, but we need a way to

How is this relevant to the discussion? Are the clock chips connected to the I2C bus too?

Quoted 1 line

Quoted 1 line

monitor the temperature and bump the fan speed if needed.

Hardware is very badly designed if the driver actually has to take care of this. Thermal management should be handled by the hardware directly. And as a matter of fact, the Analog Devices ADT7473 and the Fintek F75375S support this.

Quoted 2 lines

Quoted 2 lines

2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Do you mean that you don't have the code at all yet, or that you did not include it in this patch set? Either way, this is wrong. There is no point in asking for a review of only half of your solution. We can't comment on the relevance of your proposal without seeing how you intend to use it.

I have no plan to review your patch, sorry. You did not provide a proper description of your problem, and you didn't explain why it can't be solved with the current kernel infrastructures. Worse, you propose a brand new hwmon subsystem, but you don't even provide a description of its design, let alone an explanation of why you think this design is appropriate. And you don't show us code using it either.

All I can say after a quick look at the patch, is that you are overengineering a lot. You have enumerated all sensor properties which exist, and are trying to handle all sensor types. You have a specific need (thermal management of graphics cards), but you are already trying to provide a generic access to all hwmon attributes which exist, regardless of the needs or relevance. Do you really think you'll need information about the case intrusion status in nouveau? Seriously?

Quoted 4 lines

This is an extremely complex change just for the benefit of one driver, with a huge potential of misuse. The changes required in each driver to actually implement the API are substantial, and pretty much only add complexity to each hwmon driver with no real benefit.

I would be very curious to know how comes that the radeon driver apparently works just fine without this change, if the nouveau driver can't do without it.

My main concern is that the code you will have to add to every new hwmon driver you will want nouveau to be able to use, is likely to be larger than the code needed to access the device registers directly from nouveau. Getting a temperature value from a hwmon device is typically done with a single call to i2c_smbus_read_byte_data().

Quoted 3 lines

The cost gets even larger if one has to consider that some may want or have to to backport drivers to earlier kernel versions. This patchset would result in significant efforts to do such backports.

This will never be a good reason to reject a change, sorry. Just look at the many changes the i2c subsystem went through in the past 2 years. They make it difficult to backport i2c device drivers to older kernels, but they still happened, because they were needed. When backporting a driver, you have to deal with the history of the kernel at large, that's life.

Quoted 6 lines

For the API itself, there are lots of functions with similar parameters, and those parameters are needed in the drivers to determine which attribute is affected. A single function would have accomplished the same, as the drivers will need case statements anyway to identify the actual attribute to be read or written. What we end up here with is a large number of functions to be supported by each driver, all with pretty much the same set of arguments.

This is the kind of thing which would show up immediately if we could see a few actual implementations of the hwmon driver side of the API. In general, I would tend to agree with Guenter that exporting a dozen functions from the hwmon core driver seems just wrong, especially given the specific problem you claim you are trying to solve.

Quoted 5 lines

Quoted 4 lines

The cost gets even larger if one has to consider that some may want or have to to backport drivers to earlier kernel versions. This patchset would result in significant efforts to do such backports.

Quoted 6 lines

This will never be a good reason to reject a change, sorry. Just look at the many changes the i2c subsystem went through in the past 2 years. They make it difficult to backport i2c device drivers to older kernels, but they still happened, because they were needed. When backporting a driver, you have to deal with the history of the kernel at large, that's life.

(agree)

.. unless there is no benefit for the driver in such an API change. I am all for API changes if there are benefits for the affected drivers, and don't care about backwards compatibility. The i2c subsystem changes all resulted in much cleaner and less code, which benefits everyone.

What we have here, though, is a lot of added complexity for each driver with no benefit at all for the affected drivers. From a driver perspective, added complexity and loss of backward complexity is all we get. In that context I consider lack of backward complexity to be a valid argument. Sure, there may be other arguments to make which outweigh the concerns, but that doesn't mean the argument can not be made.

It's often the case that fan control on these cards is handled by an external hwmon chip, while the thermal diode is integrated into the chip core and therefore only readable by the PCI driver. In that case we need to be able to access the fan control registers. In other cases, cooling is implemented passively by reducing the clock on the card. In those cases the PCI driver needs to be able to obtain the temperature from off-card chips. Whether you think the hardware approach sucks or not, the reality is that we have plenty of devices where maanging the thermal state of the hardware requires a single driver to be able to access both PCI and i2c devices.

On Mon, Feb 14, 2011 at 11:23:38AM -0500, Matthew Garrett wrote:
[ ... ]

Quoted 12 lines

It's often the case that fan control on these cards is handled by an external hwmon chip, while the thermal diode is integrated into the chip core and therefore only readable by the PCI driver. In that case we need to be able to access the fan control registers. In other cases, cooling is implemented passively by reducing the clock on the card. In those cases the PCI driver needs to be able to obtain the temperature from off-card chips. Whether you think the hardware approach sucks or not, the reality is that we have plenty of devices where maanging the thermal state of the hardware requires a single driver to be able to access both PCI and i2c devices.

So what you actually want to do is to implement fan control and/or, more generically, power management, in the kernel. Fan control is so far implemented in userland, the idea being that it is too complex and not well enough understood to implement in the kernel.

So one of the questions is why you can not keep it that way - and that might include clock management if the required attributes are made available through sysfs. Once the code is stable, and if there are good arguments to move the functionality into the kernel, one may still consider doing it. By then we would hopefully also have a better understanding of requirements, such as if and to what degree interrupt and event handling support would be required, and how to organize and/or create necessary subsystems to be usable for everyone, not just for GPUs.

Quoted 14 lines · Guenter Roeck

On Mon, Feb 14, 2011 at 11:23:38AM -0500, Matthew Garrett wrote:
[ ... ]

Quoted 12 lines

It's often the case that fan control on these cards is handled by an external hwmon chip, while the thermal diode is integrated into the chip core and therefore only readable by the PCI driver. In that case we need to be able to access the fan control registers. In other cases, cooling is implemented passively by reducing the clock on the card. In those cases the PCI driver needs to be able to obtain the temperature from off-card chips. Whether you think the hardware approach sucks or not, the reality is that we have plenty of devices where maanging the thermal state of the hardware requires a single driver to be able to access both PCI and i2c devices.

Quoted 4 lines

So what you actually want to do is to implement fan control and/or, more generically, power management, in the kernel. Fan control is so far implemented in userland, the idea being that it is too complex and not well enough understood to implement in the kernel.

The kernel has generic support for thermal management. Making it work for any device is just a matter of a driver generating the appropriate thermal and cooling devices and associating them. ACPI already works this way.

Quoted 7 lines

So one of the questions is why you can not keep it that way - and that might include clock management if the required attributes are made available through sysfs. Once the code is stable, and if there are good arguments to move the functionality into the kernel, one may still consider doing it. By then we would hopefully also have a better understanding of requirements, such as if and to what degree interrupt and event handling support would be required, and how to organize and/or create necessary subsystems to be usable for everyone, not just for GPUs.

Because hardware control is the kernel's job, not userspace's. Having hardware melt just because userspace fell off a cliff isn't acceptable.

Quoted 5 lines · Matthew Garrett

The kernel has generic support for thermal management. Making it work for any device is just a matter of a driver generating the appropriate thermal and cooling devices and associating them. ACPI already works this way.

Not sure what you suggest here. Thermal devices register as hwmon devices if so configured, so having hwmon devices register as thermal devices would not work, at least not without some serious thought to prevent registration loops.

If you are looking for thermal device support, maybe the drivers in question should be implemented as thermal device drivers and provide hwmon functionality through the thermal subsystem. Did you consider that option ?

Do you plan to use/utilize the thermal subsystem, or do you plan to duplicate that functionality in the GPU driver(s) ? Guess that was one of the reasons why Jean asked for a use case of the proposed API.

If you plan to use the thermal subsystem without having to rewrite hwmon drivers, did you consider means to interconnect the hwmon subsystem with the thermal subsystem, eg by creating a means for hwmon devices to register as thermal devices ?

Quoted 3 lines

Because hardware control is the kernel's job, not userspace's. Having hardware melt just because userspace fell off a cliff isn't acceptable.

The same argument would apply to system fan control, doesn't it ?

Quoted 10 lines · Guenter Roeck

Quoted 5 lines · Matthew Garrett

The kernel has generic support for thermal management. Making it work for any device is just a matter of a driver generating the appropriate thermal and cooling devices and associating them. ACPI already works this way.

Not sure what you suggest here. Thermal devices register as hwmon devices if so configured, so having hwmon devices register as thermal devices would not work, at least not without some serious thought to prevent registration loops.

That's really an implementation detail - the worst case at present is that you'd end up with the same sensor providing data twice, but that's fixable.

Quoted 3 lines

If you are looking for thermal device support, maybe the drivers in question should be implemented as thermal device drivers and provide hwmon functionality through the thermal subsystem. Did you consider that option ?

We could definitely change the existing hwmon drivers to be thermal drivers instead, but not everything they do fits into that model.

Quoted 3 lines

Do you plan to use/utilize the thermal subsystem, or do you plan to duplicate that functionality in the GPU driver(s) ? Guess that was one of the reasons why Jean asked for a use case of the proposed API.

Use the thermal subsystem. That's why I made the ACPI thermal code generic in the first place.

Quoted 3 lines

If you plan to use the thermal subsystem without having to rewrite hwmon drivers, did you consider means to interconnect the hwmon subsystem with the thermal subsystem, eg by creating a means for hwmon devices to register as thermal devices ?

As I said, we could rework the hwmon drivers into thermal devices instead, but we'd still need a mechanism for providing the thermal device back to the registering device.

Quoted 4 lines

Quoted 3 lines

Because hardware control is the kernel's job, not userspace's. Having hardware melt just because userspace fell off a cliff isn't acceptable.

The same argument would apply to system fan control, doesn't it ?

Yes, which is why it belongs in the kernel.

Quoted 4 lines · Matthew Garrett

As I said, we could rework the hwmon drivers into thermal devices instead, but we'd still need a mechanism for providing the thermal device back to the registering device.

Not sure I understand the second part. How do you do that today for ACPI thermal devices ?

Quoted 7 lines

Quoted 4 lines

Quoted 3 lines

Because hardware control is the kernel's job, not userspace's. Having hardware melt just because userspace fell off a cliff isn't acceptable.

The same argument would apply to system fan control, doesn't it ?

Yes, which is why it belongs in the kernel.

Accepted, but that implies a generic solution, not one restricted/limited to GPUs.

Quoted 8 lines · Jean Delvare

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Martin Peres

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Quoted 3 lines

Why? You already have a way to get data out of and send commands to the I2C devices themselves (Using the i2c_smbus_* functions). Why do you insist on going through the I2C device drivers?

Because implementing support for every sensor chip when there's already code in the kernel to deal with them isn't the usual way of handling problems.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

We can already change the clocks of the card, but we need a way to

How is this relevant to the discussion? Are the clock chips connected to the I2C bus too?

It's all part of the power management setup.

Quoted 6 lines

Quoted 1 line

Quoted 1 line

monitor the temperature and bump the fan speed if needed.

Hardware is very badly designed if the driver actually has to take care of this. Thermal management should be handled by the hardware directly. And as a matter of fact, the Analog Devices ADT7473 and the Fintek F75375S support this.

It's often the case that fan control on these cards is handled by an external hwmon chip, while the thermal diode is integrated into the chip core and therefore only readable by the PCI driver. In that case we need to be able to access the fan control registers. In other cases, cooling is implemented passively by reducing the clock on the card. In those cases the PCI driver needs to be able to obtain the temperature from off-card chips. Whether you think the hardware approach sucks or not, the reality is that we have plenty of devices where maanging the thermal state of the hardware requires a single driver to be able to access both PCI and i2c devices.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Do you mean that you don't have the code at all yet, or that you did not include it in this patch set? Either way, this is wrong. There is no point in asking for a review of only half of your solution. We can't comment on the relevance of your proposal without seeing how you intend to use it.

There's no point in adapting drivers if the API is unacceptable to the upstream maintainers.

We need a mechanism for in-kernel drivers to read
and write from hwmon devices. What should the interface to do so look like? This is one proposal which would satisfy the current consumers and could potentially be used for other devices as well.

Quoted 6 lines

I have no plan to review your patch, sorry. You did not provide a proper description of your problem, and you didn't explain why it can't be solved with the current kernel infrastructures. Worse, you propose a brand new hwmon subsystem, but you don't even provide a description of its design, let alone an explanation of why you think this design is appropriate. And you don't show us code using it either.

I hope I've provided a bit more context, but to summarise:

Power and thermal management of GPUs requires that the GPU driver be able to obtain the GPU temperature and control the GPU fan. On many shipping devices, doing so requires the ability for the GPU driver to obtain values from devices attached via i2c. The kernel already has drivers for these devices and so it makes sense to use them rather than duplicating them. Adding a generic hwmon interface for in-kernel use would allow these drivers to be used for this purpose.

Quoted 7 lines

All I can say after a quick look at the patch, is that you are overengineering a lot. You have enumerated all sensor properties which exist, and are trying to handle all sensor types. You have a specific need (thermal management of graphics cards), but you are already trying to provide a generic access to all hwmon attributes which exist, regardless of the needs or relevance. Do you really think you'll need information about the case intrusion status in nouveau? Seriously?

If we're adding an in-kernel API then it makes sense for it to be generic enoguh that anyone could use it,

but there's no real problem in
just cutting out any functions that wouldn't be used by anyone as yet.

Quoted 8 lines

Quoted 4 lines

This is an extremely complex change just for the benefit of one driver, with a huge potential of misuse. The changes required in each driver to actually implement the API are substantial, and pretty much only add complexity to each hwmon driver with no real benefit.

I would be very curious to know how comes that the radeon driver apparently works just fine without this change, if the nouveau driver can't do without it.

It doesn't. Right now the fan control is left up to the card firmware, which means that it runs far faster than necessary and also means that

the card can't drop clock rates in response to reaching temperature limits.

Radeon will benefit from this just as much as nouveau.

Quoted 5 lines

My main concern is that the code you will have to add to every new hwmon driver you will want nouveau to be able to use, is likely to be larger than the code needed to access the device registers directly from nouveau. Getting a temperature value from a hwmon device is typically done with a single call to i2c_smbus_read_byte_data().

Duplicating the functionality directly in novueau (and again in radeon) just means that every time we find a bug in an hwmon driver we have to fix it in three places. Upstream is typically resistant to this kind of thing.

Quoted 6 lines

(...)
This is the kind of thing which would show up immediately if we could see a few actual implementations of the hwmon driver side of the API. In general, I would tend to agree with Guenter that exporting a dozen functions from the hwmon core driver seems just wrong, especially given the specific problem you claim you are trying to solve.

Replacing it with a read/set function with a type enum as the first argument would work as well. Would that be more reasonable?

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Matthew Garrett

The kernel has generic support for thermal management. Making it work for any device is just a matter of a driver generating the appropriate thermal and cooling devices and associating them. ACPI already works this way.

Quoted 4 lines

Not sure what you suggest here. Thermal devices register as hwmon devices if so configured, so having hwmon devices register as thermal devices would not work, at least not without some serious thought to prevent registration loops.

That's really an implementation detail - the worst case at present is that you'd end up with the same sensor providing data twice, but that's fixable.

Quoted 3 lines

If you are looking for thermal device support, maybe the drivers in question should be implemented as thermal device drivers and provide hwmon functionality through the thermal subsystem. Did you consider that option ?

We could definitely change the existing hwmon drivers to be thermal drivers instead, but not everything they do fits into that model.

Quoted 3 lines

Do you plan to use/utilize the thermal subsystem, or do you plan to duplicate that functionality in the GPU driver(s) ? Guess that was one of the reasons why Jean asked for a use case of the proposed API.

Use the thermal subsystem. That's why I made the ACPI thermal code generic in the first place.

Quoted 3 lines

If you plan to use the thermal subsystem without having to rewrite hwmon drivers, did you consider means to interconnect the hwmon subsystem with the thermal subsystem, eg by creating a means for hwmon devices to register as thermal devices ?

As I said, we could rework the hwmon drivers into thermal devices instead, but we'd still need a mechanism for providing the thermal device back to the registering device.

Quoted 4 lines

Quoted 3 lines

Because hardware control is the kernel's job, not userspace's. Having hardware melt just because userspace fell off a cliff isn't acceptable.

The same argument would apply to system fan control, doesn't it ?

Yes, which is why it belongs in the kernel.

Quoted 7 lines · Matthew Garrett

Quoted 6 lines · Guenter Roeck

Quoted 5 lines · Matthew Garrett

The kernel has generic support for thermal management. Making it work for any device is just a matter of a driver generating the appropriate thermal and cooling devices and associating them. ACPI already works this way.

Quoted 8 lines

Quoted 4 lines

Not sure what you suggest here. Thermal devices register as hwmon devices if so configured, so having hwmon devices register as thermal devices would not work, at least not without some serious thought to prevent registration loops.

That's really an implementation detail - the worst case at present is that you'd end up with the same sensor providing data twice, but that's fixable.

I agree. I tend to believe that this approach would be superior to the initial proposal for the problem at hand. The few required hwmon drivers would simply register their temperature sensors as thermal zones, and their fan control outputs as cooling devices, to the thermal subsystem. This requires no change to the hwmon core, nor to all other individual hwmon drivers.

Quoted 6 lines

Quoted 3 lines

If you are looking for thermal device support, maybe the drivers in question should be implemented as thermal device drivers and provide hwmon functionality through the thermal subsystem. Did you consider that option ?

We could definitely change the existing hwmon drivers to be thermal drivers instead, but not everything they do fits into that model.

Not instead. Additionally.

Quoted 6 lines

Quoted 3 lines

Do you plan to use/utilize the thermal subsystem, or do you plan to duplicate that functionality in the GPU driver(s) ? Guess that was one of the reasons why Jean asked for a use case of the proposed API.

Use the thermal subsystem. That's why I made the ACPI thermal code generic in the first place.

This sounds good.

Quoted 7 lines

Quoted 3 lines

If you plan to use the thermal subsystem without having to rewrite hwmon drivers, did you consider means to interconnect the hwmon subsystem with the thermal subsystem, eg by creating a means for hwmon devices to register as thermal devices ?

As I said, we could rework the hwmon drivers into thermal devices instead, but we'd still need a mechanism for providing the thermal device back to the registering device.

Correct.

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

Because hardware control is the kernel's job, not userspace's. Having hardware melt just because userspace fell off a cliff isn't acceptable.

The same argument would apply to system fan control, doesn't it ?

Yes, which is why it belongs in the kernel.

The truth is IMHO more complex than this. I tend to believe that fan speed control (or more generally thermal management) can be implemented at different levels. Think about it, it's just like CPU frequency scaling. You have kernel governors and one user-space governor for people unhappy about the in-kernel ones and/or for hardware which can't be supported by these.

Ideally, thermal management would be implemented by the hardware, and neither the kernel drivers nor user-space should have to care. Less ideally, the kernel driver has to care, but has enough knowledge to do this by itself without user intervention. The worse case is where the kernel doesn't know and the user has to configure everything manually. This is the case of many PC motherboards out there, for which users rely on the user-space fancontrol script (or anything similar.) It can be pretty useful, but I still avoid when I can. And I definitely wouldn't recommend this approach for GPUs.

So I can envision a similarly layered thermal management logic, where the kernel drivers take care of what they have to and can handle, and leave the rest to user-space (as is already the case today.) In other words, please let me be clear that I have no objection to what Matthew and Martin are trying to achieve. My objections relate to the implementation and the way to submit it for review.

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Why? You already have a way to get data out of and send commands to the I2C devices themselves (Using the i2c_smbus_* functions). Why do you insist on going through the I2C device drivers?

Quoted 1 line

Quoted 1 line

We can already change the clocks of the card, but we need a way to

How is this relevant to the discussion? Are the clock chips connected to the I2C bus too?

Quoted 1 line

Quoted 1 line

monitor the temperature and bump the fan speed if needed.

Hardware is very badly designed if the driver actually has to take care of this. Thermal management should be handled by the hardware directly. And as a matter of fact, the Analog Devices ADT7473 and the Fintek F75375S support this.

Quoted 3 lines

Quoted 3 lines

Another problem with letting users mess with the i2c driver by themselves is that some cards use the i2c driver for fan management while others don't. This is why I would like to introduce nouveau as an

I guess you mean I2C device, not i2c driver. You will have to be precise in your wording if you want others to understand where you are going.

When the I2C device isn't used for fan management, how is it done?

Quoted 3 lines

Quoted 3 lines

hwmon driver, exporting the temperature, fan management and clock speeds so as we can use the thermal zone to monitor the temperature and react when needed.

It only makes sense to instantiate a hwmon device from nouveau directly if the temperature sensor or the fan management is _not_ done by an I2C device. So it seems unrelated with your patch. Why are you mentioning it then?

And clock speeds don't have anything to do with hwmon, BTW.

Quoted 9 lines

Quoted 9 lines

So far, we use:
- w83l785ts
- w83781d
- adt7473 (most common one)
- f75375
- lm99

With the help of Matthew Garret, I updated his previous proposal for an in-kernel API for hwmon. The patch should apply cleanly on Linux

I can't remember this proposal. A link would be appreciated.

Quoted 2 lines

Quoted 2 lines

2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Do you mean that you don't have the code at all yet, or that you did not include it in this patch set? Either way, this is wrong. There is no point in asking for a review of only half of your solution. We can't comment on the relevance of your proposal without seeing how you intend to use it.

Quoted 1 line

Quoted 1 line

Looking forward to your review and feedback.

I have no plan to review your patch, sorry. You did not provide a proper description of your problem, and you didn't explain why it can't be solved with the current kernel infrastructures. Worse, you propose a brand new hwmon subsystem, but you don't even provide a description of its design, let alone an explanation of why you think this design is appropriate. And you don't show us code using it either.

All I can say after a quick look at the patch, is that you are overengineering a lot. You have enumerated all sensor properties which exist, and are trying to handle all sensor types. You have a specific need (thermal management of graphics cards), but you are already trying to provide a generic access to all hwmon attributes which exist, regardless of the needs or relevance. Do you really think you'll need information about the case intrusion status in nouveau? Seriously?

Quoted 5 lines

Quoted 5 lines

From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001
From: Martin Peres <[email protected]>
Date: Sun, 13 Feb 2011 11:35:17 +0100
Subject: [PATCH] hwmon API update

Quoted 8 lines

Quoted 3 lines

Original creator: Matthew Garrett <[email protected]>

Signed-off-by Martin Peres <[email protected]>

This is an extremely complex change just for the benefit of one driver, with a huge potential of misuse. The changes required in each driver to actually implement the API are substantial, and pretty much only add complexity to each hwmon driver with no real benefit.

I would be very curious to know how comes that the radeon driver apparently works just fine without this change, if the nouveau driver can't do without it.

My main concern is that the code you will have to add to every new hwmon driver you will want nouveau to be able to use, is likely to be larger than the code needed to access the device registers directly from nouveau. Getting a temperature value from a hwmon device is typically done with a single call to i2c_smbus_read_byte_data().

Quoted 3 lines

The cost gets even larger if one has to consider that some may want or have to to backport drivers to earlier kernel versions. This patchset would result in significant efforts to do such backports.

This will never be a good reason to reject a change, sorry. Just look at the many changes the i2c subsystem went through in the past 2 years. They make it difficult to backport i2c device drivers to older kernels, but they still happened, because they were needed. When backporting a driver, you have to deal with the history of the kernel at large, that's life.

Quoted 6 lines

For the API itself, there are lots of functions with similar parameters, and those parameters are needed in the drivers to determine which attribute is affected. A single function would have accomplished the same, as the drivers will need case statements anyway to identify the actual attribute to be read or written. What we end up here with is a large number of functions to be supported by each driver, all with pretty much the same set of arguments.

This is the kind of thing which would show up immediately if we could see a few actual implementations of the hwmon driver side of the API. In general, I would tend to agree with Guenter that exporting a dozen functions from the hwmon core driver seems just wrong, especially given the specific problem you claim you are trying to solve.

Quoted 10 lines

I don't know what current thinking is about kernel size increases, but it looks like this patch will result in quite significant kernel size increase (some 18*8 = 144 bytes per driver for all the pointers, plus the actual functions, adds up to a lot). Again this would be with no benefit for most of the users of the hwmon subsystem. Sure, one can argue that the size increases will only occur if the drivers are actually loaded, but that is a pretty weak argument since the code size increase will still show up in each driver.

In summary I am not in favor for this change. Maybe Jean thinks differently, but for my part I don't plan to approve it.

I don't plan to approve it either, at least not in its current state. As I said above already, I want a complete description of the problem first, an explanation of why the change is needed, why a more lightweight solution wouldn't do, and why nouveau needs it when radeon doesn't. And I want to see actual implementations of the API on both sides.

Sorry but you can't push for a new API affecting 100 drivers without justifying everything you do. An API with no implementers and no users is not how you'll convince me.

Hello all,

let me revive this discussion a bit. After reading some things about the matter I think the way to go is to use Intels thermal framework. It should be easy to add the needed temp_get and fan_set functions to the affected hwmon drivers.

But then one problem still persists: when the hwmon driver probes a device it automatically creates the sysfs entries. This is unintended behaviour from nouveau's point of view. The sensor value is not the real temperature value; it has to be scaled with some factor and an offset hard-coded in the video bios tables. This scaling could only be done on nouveau's side, so the hwmon driver is exposing an interface with wrong values to the userspace.

We need a way to use the hwmon i2c driver without exposing this interface. Do you have any preferred way how we could achieve this?

I see two ways to do this:
1. Sort out if we need the sysfs entries in the probe function. This could be done with some name postfix in the board_info. Downside: adds some noise to this function.

2. Creating a new module which only exposes the thermal framework API. This leads to code duplication and we end up with two drivers for the same i2c monitoring chip. On the other hand this _could_ lead to cleaner code and we could move this part to drivers/thermal instead of drivers/hwmon. I don't like the code duplication, do you see any chance to avoid this in case this is the way to go?

Please comment on my thoughts, we need your feedback to bring up an API everyone could agree on. I'm willing to hack together a prototype over the next week, but want to avoid running in a completely wrong direction.

-- Lucas

Am Sonntag, den 13.02.2011, 23:08 +0100 schrieb Jean Delvare:

Quoted 153 lines · Guenter Roeck

Quoted 6 lines · Martin Peres

Quoted 5 lines

Hi,

I am working on power management on the nouveau driver and I need a way to get data out of and send commands to the i2c drivers from the kernel space.

Why? You already have a way to get data out of and send commands to the I2C devices themselves (Using the i2c_smbus_* functions). Why do you insist on going through the I2C device drivers?

Quoted 1 line

Quoted 1 line

We can already change the clocks of the card, but we need a way to

How is this relevant to the discussion? Are the clock chips connected to the I2C bus too?

Quoted 1 line

Quoted 1 line

monitor the temperature and bump the fan speed if needed.

Hardware is very badly designed if the driver actually has to take care of this. Thermal management should be handled by the hardware directly. And as a matter of fact, the Analog Devices ADT7473 and the Fintek F75375S support this.

Quoted 3 lines

Quoted 3 lines

Another problem with letting users mess with the i2c driver by themselves is that some cards use the i2c driver for fan management while others don't. This is why I would like to introduce nouveau as an

I guess you mean I2C device, not i2c driver. You will have to be precise in your wording if you want others to understand where you are going.

When the I2C device isn't used for fan management, how is it done?

Quoted 3 lines

Quoted 3 lines

hwmon driver, exporting the temperature, fan management and clock speeds so as we can use the thermal zone to monitor the temperature and react when needed.

It only makes sense to instantiate a hwmon device from nouveau directly if the temperature sensor or the fan management is _not_ done by an I2C device. So it seems unrelated with your patch. Why are you mentioning it then?

And clock speeds don't have anything to do with hwmon, BTW.

Quoted 9 lines

Quoted 9 lines

So far, we use:
- w83l785ts
- w83781d
- adt7473 (most common one)
- f75375
- lm99

With the help of Matthew Garret, I updated his previous proposal for an in-kernel API for hwmon. The patch should apply cleanly on Linux

I can't remember this proposal. A link would be appreciated.

Quoted 2 lines

Quoted 2 lines

2.6.38-rc4. This patch only provides the API, no modification to the drivers has been completed yet.

Do you mean that you don't have the code at all yet, or that you did not include it in this patch set? Either way, this is wrong. There is no point in asking for a review of only half of your solution. We can't comment on the relevance of your proposal without seeing how you intend to use it.

Quoted 1 line

Quoted 1 line

Looking forward to your review and feedback.

I have no plan to review your patch, sorry. You did not provide a proper description of your problem, and you didn't explain why it can't be solved with the current kernel infrastructures. Worse, you propose a brand new hwmon subsystem, but you don't even provide a description of its design, let alone an explanation of why you think this design is appropriate. And you don't show us code using it either.

All I can say after a quick look at the patch, is that you are overengineering a lot. You have enumerated all sensor properties which exist, and are trying to handle all sensor types. You have a specific need (thermal management of graphics cards), but you are already trying to provide a generic access to all hwmon attributes which exist, regardless of the needs or relevance. Do you really think you'll need information about the case intrusion status in nouveau? Seriously?

Quoted 5 lines

Quoted 5 lines

From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001
From: Martin Peres <[email protected]>
Date: Sun, 13 Feb 2011 11:35:17 +0100
Subject: [PATCH] hwmon API update

Quoted 8 lines

Quoted 3 lines

Original creator: Matthew Garrett <[email protected]>

Signed-off-by Martin Peres <[email protected]>

This is an extremely complex change just for the benefit of one driver, with a huge potential of misuse. The changes required in each driver to actually implement the API are substantial, and pretty much only add complexity to each hwmon driver with no real benefit.

I would be very curious to know how comes that the radeon driver apparently works just fine without this change, if the nouveau driver can't do without it.

My main concern is that the code you will have to add to every new hwmon driver you will want nouveau to be able to use, is likely to be larger than the code needed to access the device registers directly from nouveau. Getting a temperature value from a hwmon device is typically done with a single call to i2c_smbus_read_byte_data().

Quoted 3 lines

The cost gets even larger if one has to consider that some may want or have to to backport drivers to earlier kernel versions. This patchset would result in significant efforts to do such backports.

This will never be a good reason to reject a change, sorry. Just look at the many changes the i2c subsystem went through in the past 2 years. They make it difficult to backport i2c device drivers to older kernels, but they still happened, because they were needed. When backporting a driver, you have to deal with the history of the kernel at large, that's life.

Quoted 6 lines

For the API itself, there are lots of functions with similar parameters, and those parameters are needed in the drivers to determine which attribute is affected. A single function would have accomplished the same, as the drivers will need case statements anyway to identify the actual attribute to be read or written. What we end up here with is a large number of functions to be supported by each driver, all with pretty much the same set of arguments.

This is the kind of thing which would show up immediately if we could see a few actual implementations of the hwmon driver side of the API. In general, I would tend to agree with Guenter that exporting a dozen functions from the hwmon core driver seems just wrong, especially given the specific problem you claim you are trying to solve.

Quoted 10 lines

I don't know what current thinking is about kernel size increases, but it looks like this patch will result in quite significant kernel size increase (some 18*8 = 144 bytes per driver for all the pointers, plus the actual functions, adds up to a lot). Again this would be with no benefit for most of the users of the hwmon subsystem. Sure, one can argue that the size increases will only occur if the drivers are actually loaded, but that is a pretty weak argument since the code size increase will still show up in each driver.

In summary I am not in favor for this change. Maybe Jean thinks differently, but for my part I don't plan to approve it.

I don't plan to approve it either, at least not in its current state. As I said above already, I want a complete description of the problem first, an explanation of why the change is needed, why a more lightweight solution wouldn't do, and why nouveau needs it when radeon doesn't. And I want to see actual implementations of the API on both sides.

Sorry but you can't push for a new API affecting 100 drivers without justifying everything you do. An API with no implementers and no users is not how you'll convince me.

Quoted 15 lines

Hello all,

let me revive this discussion a bit. After reading some things about the matter I think the way to go is to use Intels thermal framework. It should be easy to add the needed temp_get and fan_set functions to the affected hwmon drivers.

But then one problem still persists: when the hwmon driver probes a device it automatically creates the sysfs entries. This is unintended behaviour from nouveau's point of view. The sensor value is not the real temperature value; it has to be scaled with some factor and an offset hard-coded in the video bios tables. This scaling could only be done on nouveau's side, so the hwmon driver is exposing an interface with wrong values to the userspace.

Quoted 3 lines

We need a way to use the hwmon i2c driver without exposing this interface. Do you have any preferred way how we could achieve this?

The whole point of the hwmon subsystem is to expose hardware monitoring attributes to userspace. A hwmon driver without sysfs attributes does not make sense.

Guenter

Quoted 13 lines

I see two ways to do this:
1. Sort out if we need the sysfs entries in the probe function. This could be done with some name postfix in the board_info. Downside: adds some noise to this function.

2. Creating a new module which only exposes the thermal framework API. This leads to code duplication and we end up with two drivers for the same i2c monitoring chip. On the other hand this _could_ lead to cleaner code and we could move this part to drivers/thermal instead of drivers/hwmon. I don't like the code duplication, do you see any chance to avoid this in case this is the way to go?

Quoted 6 lines

Please comment on my thoughts, we need your feedback to bring up an API everyone could agree on. I'm willing to hack together a prototype over the next week, but want to avoid running in a completely wrong direction.

-- Lucas

Quoted 15 lines

Hello all,

let me revive this discussion a bit. After reading some things about the matter I think the way to go is to use Intels thermal framework. It should be easy to add the needed temp_get and fan_set functions to the affected hwmon drivers.

But then one problem still persists: when the hwmon driver probes a device it automatically creates the sysfs entries. This is unintended behaviour from nouveau's point of view. The sensor value is not the real temperature value; it has to be scaled with some factor and an offset hard-coded in the video bios tables. This scaling could only be done on nouveau's side, so the hwmon driver is exposing an interface with wrong values to the userspace.

Quoted 3 lines

We need a way to use the hwmon i2c driver without exposing this interface. Do you have any preferred way how we could achieve this?

The whole point of the hwmon subsystem is to expose hardware monitoring attributes to userspace. A hwmon driver without sysfs attributes does not make sense.