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
On Sun, Feb 13, 2011 at 07:18:44AM -0500, Martin Peres wrote:> 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 <martin.peres at ensi-bourges.fr> > Date: Sun, 13 Feb 2011 11:35:17 +0100 > Subject: [PATCH] hwmon API update > > Original creator: Matthew Garrett <mjg at redhat.com> > > Signed-off-by: Martin Peres <martin.peres at ensi-bourges.fr>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
Le 13/02/2011 18:16, Guenter Roeck a ?crit :> On Sun, Feb 13, 2011 at 07:18:44AM -0500, Martin Peres wrote: >> 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<martin.peres at ensi-bourges.fr> >> Date: Sun, 13 Feb 2011 11:35:17 +0100 >> Subject: [PATCH] hwmon API update >> >> Original creator: Matthew Garrett<mjg at redhat.com> >> >> Signed-off-by: Martin Peres<martin.peres at ensi-bourges.fr> > 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. > > GuenterActually, it is not completely true. This API isn't mandatory for the drivers to implement. We could only modify the drivers we need in nouveau and leave the others untouched but this is only good for as a transition from the sysfs-only interface to the new interface. I agree that changing hwmon in the way we are asking is a big change in philosophy, but what are you suggesting? We can't just re-implement the needed i2c drivers in nouveau and the only way we can access the already existing i2c drivers is through sysfs. The real question is why hwmon only is targeted for the userland? Another question is, why is the actual code of the drivers buried so deep inside the implementation details of the sysfs interface (this is what makes it so painful to update)? Actually, this proposal could save space as once this interface is adopted by some drivers, all the sysfs-related code could be shared in hwmon.c. Another proposal could be to access the drivers through sysfs, but I don't know if it is possible and I think it would be abusing the sysfs interface anyway. I think you now understand our situation a bit better, do you have any suggestion? I really wish to find an agreement on this as not sharing the code is not an option for me. Martin
On Sun, 13 Feb 2011 09:16:40 -0800, Guenter Roeck wrote:> On Sun, Feb 13, 2011 at 07:18:44AM -0500, Martin Peres wrote: > > 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?> > We can already change the clocks of the card, but we need a way toHow is this relevant to the discussion? Are the clock chips connected to the I2C bus too?> > 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.> > 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 anI 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?> > 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.> > 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 LinuxI can't remember this proposal. A link would be appreciated.> > 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.> > 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?> > From 059b647b7b8bd98c04cf48b4062048b8ae963593 Mon Sep 17 00:00:00 2001 > > From: Martin Peres <martin.peres at ensi-bourges.fr> > > Date: Sun, 13 Feb 2011 11:35:17 +0100 > > Subject: [PATCH] hwmon API update > > > > Original creator: Matthew Garrett <mjg at redhat.com> > > > > Signed-off-by: Martin Peres <martin.peres at ensi-bourges.fr> > > 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().> 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.> 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.> 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. -- Jean Delvare