Dependency chain of new thermal driver

Dependency chain of new thermal driver

[Jean Delvare]

Hi Rui,

I am testing your new thermal driver and I am not very happy with the
dependencies introduced by this driver. The ACPI thermal driver selects
the generic thermal driver, which in turn selects the hwmon base
module. As the generic thermal driver's configuration option is a
boolean, this means that as soon as one selects the ACPI thermal driver
(built-in or modular), the hwmon thermal driver has to be built into
the kernel. This is a problem especially when both the ACPI_THERMAL and
THERMAL options default to y.

I fail to see why we are using select at all. The ACPI thermal driver
clearly works without the generic thermal driver (even though the
generic interface is preferred now.) Likewise, the generic thermal
interface driver doesn't need the hwmon base module to work; the hwmon
interface is only an extension, so it should be possible to build the
generic thermal driver without hwmon support. On top of that, I really
would like to be able to build the generic thermal driver as a module.

One of the reasons why I would like this to change is this bug:
https://bugzilla.redhat.com/show_bug.cgi?id=437637
http://bugzilla.kernel.org/show_bug.cgi?id=10259
(At the moment I think both reports are the exact same bug.)

We will have to fix this bug of course (not sure how...) but the fact
that the users can't temporarily remove the generic thermal driver is a
problem both for bug investigation and for working around the bug until
it's fixed. I really would like to be able to tell the user "rmmod this
module until we come up with a fix", but right now I can't.

A more modular setup would give us much more flexibility both at build
time and when bugs are reported. Please think about it.

-- 
Jean Delvare

[PATCH] Re: Dependency chain of new thermal driver

[Len Brown]

On Sunday 16 March 2008, Jean Delvare wrote:
> Hi Rui,
> 
> I am testing your new thermal driver and I am not very happy with the
> dependencies introduced by this driver. The ACPI thermal driver selects
> the generic thermal driver, which in turn selects the hwmon base
> module. As the generic thermal driver's configuration option is a
> boolean, this means that as soon as one selects the ACPI thermal driver
> (built-in or modular), the hwmon thermal driver has to be built into
> the kernel. This is a problem especially when both the ACPI_THERMAL and
> THERMAL options default to y.
> 
> I fail to see why we are using select at all. The ACPI thermal driver
> clearly works without the generic thermal driver (even though the
> generic interface is preferred now.) Likewise, the generic thermal
> interface driver doesn't need the hwmon base module to work; the hwmon
> interface is only an extension, so it should be possible to build the
> generic thermal driver without hwmon support. On top of that, I really
> would like to be able to build the generic thermal driver as a module.
> 
> One of the reasons why I would like this to change is this bug:
> https://bugzilla.redhat.com/show_bug.cgi?id=437637
> http://bugzilla.kernel.org/show_bug.cgi?id=10259
> (At the moment I think both reports are the exact same bug.)
> 
> We will have to fix this bug of course (not sure how...) but the fact
> that the users can't temporarily remove the generic thermal driver is a
> problem both for bug investigation and for working around the bug until
> it's fixed. I really would like to be able to tell the user "rmmod this
> module until we come up with a fix", but right now I can't.
> 
> A more modular setup would give us much more flexibility both at build
> time and when bugs are reported. Please think about it.
> 

You are absolutely right -- this is gone from 2.6.25,
lets get it right in 2.6.26.

BTW. speaking of 2.6.26....
the patch below might become helpful.  For it would
be nice if at build-time we could just have hwmon_device_register()
fail, rather than put a bunch of #ifdef HWMON in our code.

thanks,
-Len

Signed-off-by: Len Brown <len.brown@intel.com>

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index 6b6ee70..68968cc 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -16,9 +16,13 @@
 
 #include <linux/device.h>
 
+#ifdef CONFIG_HWMON
 struct device *hwmon_device_register(struct device *dev);
-
 void hwmon_device_unregister(struct device *dev);
+#else
+static struct device *hwmon_device_register(struct device *dev) { return ERR_PTR(-ENODEV); };
+static void hwmon_device_unregister(struct device *dev) { return; };
+#endif
 
 /* Scale user input to sensible values */
 static inline int SENSORS_LIMIT(long value, long low, long high)

Re: Dependency chain of new thermal driver

[Jean Delvare]

Hi Len,

On Tue, 18 Mar 2008 01:31:30 -0400, Len Brown wrote:
> On Sunday 16 March 2008, Jean Delvare wrote:
> > Hi Rui,
> > 
> > I am testing your new thermal driver and I am not very happy with the
> > dependencies introduced by this driver. The ACPI thermal driver selects
> > the generic thermal driver, which in turn selects the hwmon base
> > module. As the generic thermal driver's configuration option is a
> > boolean, this means that as soon as one selects the ACPI thermal driver
> > (built-in or modular), the hwmon thermal driver has to be built into
> > the kernel. This is a problem especially when both the ACPI_THERMAL and
> > THERMAL options default to y.
> > 
> > I fail to see why we are using select at all. The ACPI thermal driver
> > clearly works without the generic thermal driver (even though the
> > generic interface is preferred now.) Likewise, the generic thermal
> > interface driver doesn't need the hwmon base module to work; the hwmon
> > interface is only an extension, so it should be possible to build the
> > generic thermal driver without hwmon support. On top of that, I really
> > would like to be able to build the generic thermal driver as a module.
> > 
> > One of the reasons why I would like this to change is this bug:
> > https://bugzilla.redhat.com/show_bug.cgi?id=437637
> > http://bugzilla.kernel.org/show_bug.cgi?id=10259
> > (At the moment I think both reports are the exact same bug.)
> > 
> > We will have to fix this bug of course (not sure how...) but the fact
> > that the users can't temporarily remove the generic thermal driver is a
> > problem both for bug investigation and for working around the bug until
> > it's fixed. I really would like to be able to tell the user "rmmod this
> > module until we come up with a fix", but right now I can't.
> > 
> > A more modular setup would give us much more flexibility both at build
> > time and when bugs are reported. Please think about it.
> 
> You are absolutely right -- this is gone from 2.6.25,
> lets get it right in 2.6.26.

Great, thanks.

> BTW. speaking of 2.6.26....
> the patch below might become helpful.  For it would
> be nice if at build-time we could just have hwmon_device_register()
> fail, rather than put a bunch of #ifdef HWMON in our code.
> 
> thanks,
> -Len
> 
> Signed-off-by: Len Brown <len.brown@intel.com>
> 
> diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
> index 6b6ee70..68968cc 100644
> --- a/include/linux/hwmon.h
> +++ b/include/linux/hwmon.h
> @@ -16,9 +16,13 @@
>  
>  #include <linux/device.h>
>  
> +#ifdef CONFIG_HWMON
>  struct device *hwmon_device_register(struct device *dev);
> -
>  void hwmon_device_unregister(struct device *dev);
> +#else
> +static struct device *hwmon_device_register(struct device *dev) { return ERR_PTR(-ENODEV); };
> +static void hwmon_device_unregister(struct device *dev) { return; };
> +#endif
>  
>  /* Scale user input to sensible values */
>  static inline int SENSORS_LIMIT(long value, long low, long high)

I'm not sure it's such a good idea. Registering a hwmon device is not
only about calling hwmon_device_register(), it's also about creating
all the sysfs attribute files and initializing everything that's needed
for these files. There's already one driver which optionally registers
a hwmon device (drivers/input/touchscreen/ads7846.c) and looking at the
code, the patch above wouldn't really help. I think it only makes sense
to handle the conditional in <linux/hwmon.h> if (almost) everything can
be handled there and it is transparent to the drivers. Otherwise I'd say
it's not worth it, as we will still need #ifdefs in the driver code.

-- 
Jean Delvare