2011-04-26 14:56:13

by Jean Delvare

[permalink] [raw]
Subject: [PATCH v2 0/3] thermal: Cleanups

[PATCH 1/3] thermal: Hide CONFIG_THERMAL_HWMON
[PATCH 2/3] thermal: Split hwmon lookup to a separate function
[PATCH 3/3] thermal: Make THERMAL_HWMON implementation fully internal

--
Jean Delvare


2011-04-26 15:02:16

by Jean Delvare

[permalink] [raw]
Subject: [PATCH v2 1/3] thermal: Hide CONFIG_THERMAL_HWMON

It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
Anybody running a kernel >= 2.6.40 would also be running a recent
enough version of lm-sensors.

Actually having CONFIG_THERMAL_HWMON is pretty convenient so instead
of dopping it, we keep it but hide it.

Signed-off-by: Jean Delvare <[email protected]>
Cc: Rene Herman <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Guenter Roeck <[email protected]>
---
Documentation/feature-removal-schedule.txt | 9 ---------
drivers/thermal/Kconfig | 8 ++------
2 files changed, 2 insertions(+), 15 deletions(-)

--- linux-2.6.39-rc4.orig/Documentation/feature-removal-schedule.txt 2011-04-25 16:16:10.000000000 +0200
+++ linux-2.6.39-rc4/Documentation/feature-removal-schedule.txt 2011-04-26 09:29:09.000000000 +0200
@@ -295,15 +295,6 @@ Who: Ravikiran Thirumalai <kiran@scalex8

---------------------------

-What: CONFIG_THERMAL_HWMON
-When: January 2009
-Why: This option was introduced just to allow older lm-sensors userspace
- to keep working over the upgrade to 2.6.26. At the scheduled time of
- removal fixed lm-sensors (2.x or 3.x) should be readily available.
-Who: Rene Herman <[email protected]>
-
----------------------------
-
What: Code that is now under CONFIG_WIRELESS_EXT_SYSFS
(in net/core/net-sysfs.c)
When: After the only user (hal) has seen a release with the patches
--- linux-2.6.39-rc4.orig/drivers/thermal/Kconfig 2011-04-25 16:16:10.000000000 +0200
+++ linux-2.6.39-rc4/drivers/thermal/Kconfig 2011-04-26 09:32:30.000000000 +0200
@@ -14,11 +14,7 @@ menuconfig THERMAL
If you want this support, you should say Y or M here.

config THERMAL_HWMON
- bool "Hardware monitoring support"
+ bool
depends on THERMAL
depends on HWMON=y || HWMON=THERMAL
- help
- The generic thermal sysfs driver's hardware monitoring support
- requires a 2.10.7/3.0.2 or later lm-sensors userspace.
-
- Say Y if your user-space is new enough.
+ default y

--
Jean Delvare

2011-04-26 15:03:01

by Jean Delvare

[permalink] [raw]
Subject: [PATCH v2 2/3] thermal: Split hwmon lookup to a separate function

We'll soon need to reuse it.

Signed-off-by: Jean Delvare <[email protected]>
Cc: Rene Herman <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Guenter Roeck <[email protected]>
---
drivers/thermal/thermal_sys.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)

--- linux-2.6.39-rc4.orig/drivers/thermal/thermal_sys.c 2011-04-25 14:40:17.000000000 +0200
+++ linux-2.6.39-rc4/drivers/thermal/thermal_sys.c 2011-04-25 14:44:52.000000000 +0200
@@ -469,22 +469,35 @@ temp_crit_show(struct device *dev, struc
}


-static int
-thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
+static struct thermal_hwmon_device *
+thermal_hwmon_lookup_by_type(const struct thermal_zone_device *tz)
{
struct thermal_hwmon_device *hwmon;
- int new_hwmon_device = 1;
- int result;

mutex_lock(&thermal_list_lock);
list_for_each_entry(hwmon, &thermal_hwmon_list, node)
if (!strcmp(hwmon->type, tz->type)) {
- new_hwmon_device = 0;
mutex_unlock(&thermal_list_lock);
- goto register_sys_interface;
+ return hwmon;
}
mutex_unlock(&thermal_list_lock);

+ return NULL;
+}
+
+static int
+thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
+{
+ struct thermal_hwmon_device *hwmon;
+ int new_hwmon_device = 1;
+ int result;
+
+ hwmon = thermal_hwmon_lookup_by_type(tz);
+ if (hwmon) {
+ new_hwmon_device = 0;
+ goto register_sys_interface;
+ }
+
hwmon = kzalloc(sizeof(struct thermal_hwmon_device), GFP_KERNEL);
if (!hwmon)
return -ENOMEM;

--
Jean Delvare

2011-04-26 15:04:16

by Jean Delvare

[permalink] [raw]
Subject: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

THERMAL_HWMON is implemented inside the thermal_sys driver and has no
effect on drivers implementing thermal zones, so they shouldn't see
anything related to it in <linux/thermal.h>. Making the THERMAL_HWMON
implementation fully internal has two advantages beyond the cleaner
design:
* This avoids rebuilding all thermal drivers if the THERMAL_HWMON
implementation changes, or if CONFIG_THERMAL_HWMON gets enabled or
disabled.
* This avoids breaking the thermal kABI in these cases too, which
should make distributions happy.

The only drawback I can see is slightly higher memory fragmentation,
as the number of kzalloc() calls will increase by one per thermal zone.
But I doubt it will be a problem in practice, as I've never seen a
system with more than two thermal zones.

Signed-off-by: Jean Delvare <[email protected]>
Cc: Rene Herman <[email protected]>
Cc: Len Brown <[email protected]>
Cc: Guenter Roeck <[email protected]>
---
* If memory fragmentation is really a concern to anyone, it would be
possible to save one kalloc for the first temperature input of each
zone type, as the price of slightly more complex code.

* Removal code path is untested, as I have never been able to unload
the thermal_sys module on any of my systems. Something is pinning it
and I have no idea what it is.

drivers/thermal/thermal_sys.c | 116 ++++++++++++++++++++++++++++++++---------
include/linux/thermal.h | 22 -------
2 files changed, 91 insertions(+), 47 deletions(-)

--- linux-2.6.39-rc4.orig/drivers/thermal/thermal_sys.c 2011-04-26 10:30:29.000000000 +0200
+++ linux-2.6.39-rc4/drivers/thermal/thermal_sys.c 2011-04-26 11:05:38.000000000 +0200
@@ -420,6 +420,29 @@ thermal_cooling_device_trip_point_show(s

/* hwmon sys I/F */
#include <linux/hwmon.h>
+
+/* thermal zone devices with the same type share one hwmon device */
+struct thermal_hwmon_device {
+ char type[THERMAL_NAME_LENGTH];
+ struct device *device;
+ int count;
+ struct list_head tz_list;
+ struct list_head node;
+};
+
+struct thermal_hwmon_attr {
+ struct device_attribute attr;
+ char name[16];
+};
+
+/* one temperature input for each thermal zone */
+struct thermal_hwmon_temp {
+ struct list_head hwmon_node;
+ struct thermal_zone_device *tz;
+ struct thermal_hwmon_attr temp_input; /* hwmon sys attr */
+ struct thermal_hwmon_attr temp_crit; /* hwmon sys attr */
+};
+
static LIST_HEAD(thermal_hwmon_list);

static ssize_t
@@ -437,9 +460,10 @@ temp_input_show(struct device *dev, stru
int ret;
struct thermal_hwmon_attr *hwmon_attr
= container_of(attr, struct thermal_hwmon_attr, attr);
- struct thermal_zone_device *tz
- = container_of(hwmon_attr, struct thermal_zone_device,
+ struct thermal_hwmon_temp *temp
+ = container_of(hwmon_attr, struct thermal_hwmon_temp,
temp_input);
+ struct thermal_zone_device *tz = temp->tz;

ret = tz->ops->get_temp(tz, &temperature);

@@ -455,9 +479,10 @@ temp_crit_show(struct device *dev, struc
{
struct thermal_hwmon_attr *hwmon_attr
= container_of(attr, struct thermal_hwmon_attr, attr);
- struct thermal_zone_device *tz
- = container_of(hwmon_attr, struct thermal_zone_device,
+ struct thermal_hwmon_temp *temp
+ = container_of(hwmon_attr, struct thermal_hwmon_temp,
temp_crit);
+ struct thermal_zone_device *tz = temp->tz;
long temperature;
int ret;

@@ -485,10 +510,29 @@ thermal_hwmon_lookup_by_type(const struc
return NULL;
}

+/* Find the temperature input matching a given thermal zone */
+static struct thermal_hwmon_temp *
+thermal_hwmon_lookup_temp(const struct thermal_hwmon_device *hwmon,
+ const struct thermal_zone_device *tz)
+{
+ struct thermal_hwmon_temp *temp;
+
+ mutex_lock(&thermal_list_lock);
+ list_for_each_entry(temp, &hwmon->tz_list, hwmon_node)
+ if (temp->tz == tz) {
+ mutex_unlock(&thermal_list_lock);
+ return temp;
+ }
+ mutex_unlock(&thermal_list_lock);
+
+ return NULL;
+}
+
static int
thermal_add_hwmon_sysfs(struct thermal_zone_device *tz)
{
struct thermal_hwmon_device *hwmon;
+ struct thermal_hwmon_temp *temp = NULL;
int new_hwmon_device = 1;
int result;

@@ -515,30 +559,36 @@ thermal_add_hwmon_sysfs(struct thermal_z
goto unregister_hwmon_device;

register_sys_interface:
- tz->hwmon = hwmon;
+ temp = kzalloc(sizeof(struct thermal_hwmon_temp), GFP_KERNEL);
+ if (!temp) {
+ result = -ENOMEM;
+ goto unregister_hwmon_device;
+ }
+
+ temp->tz = tz;
hwmon->count++;

- snprintf(tz->temp_input.name, THERMAL_NAME_LENGTH,
+ snprintf(temp->temp_input.name, THERMAL_NAME_LENGTH,
"temp%d_input", hwmon->count);
- tz->temp_input.attr.attr.name = tz->temp_input.name;
- tz->temp_input.attr.attr.mode = 0444;
- tz->temp_input.attr.show = temp_input_show;
- sysfs_attr_init(&tz->temp_input.attr.attr);
- result = device_create_file(hwmon->device, &tz->temp_input.attr);
+ temp->temp_input.attr.attr.name = temp->temp_input.name;
+ temp->temp_input.attr.attr.mode = 0444;
+ temp->temp_input.attr.show = temp_input_show;
+ sysfs_attr_init(&temp->temp_input.attr.attr);
+ result = device_create_file(hwmon->device, &temp->temp_input.attr);
if (result)
goto unregister_hwmon_device;

if (tz->ops->get_crit_temp) {
unsigned long temperature;
if (!tz->ops->get_crit_temp(tz, &temperature)) {
- snprintf(tz->temp_crit.name, THERMAL_NAME_LENGTH,
+ snprintf(temp->temp_crit.name, THERMAL_NAME_LENGTH,
"temp%d_crit", hwmon->count);
- tz->temp_crit.attr.attr.name = tz->temp_crit.name;
- tz->temp_crit.attr.attr.mode = 0444;
- tz->temp_crit.attr.show = temp_crit_show;
- sysfs_attr_init(&tz->temp_crit.attr.attr);
+ temp->temp_crit.attr.attr.name = temp->temp_crit.name;
+ temp->temp_crit.attr.attr.mode = 0444;
+ temp->temp_crit.attr.show = temp_crit_show;
+ sysfs_attr_init(&temp->temp_crit.attr.attr);
result = device_create_file(hwmon->device,
- &tz->temp_crit.attr);
+ &temp->temp_crit.attr);
if (result)
goto unregister_hwmon_device;
}
@@ -547,14 +597,15 @@ thermal_add_hwmon_sysfs(struct thermal_z
mutex_lock(&thermal_list_lock);
if (new_hwmon_device)
list_add_tail(&hwmon->node, &thermal_hwmon_list);
- list_add_tail(&tz->hwmon_node, &hwmon->tz_list);
+ list_add_tail(&temp->hwmon_node, &hwmon->tz_list);
mutex_unlock(&thermal_list_lock);

return 0;

unregister_hwmon_device:
- device_remove_file(hwmon->device, &tz->temp_crit.attr);
- device_remove_file(hwmon->device, &tz->temp_input.attr);
+ device_remove_file(hwmon->device, &temp->temp_crit.attr);
+ device_remove_file(hwmon->device, &temp->temp_input.attr);
+ kfree(temp);
if (new_hwmon_device) {
device_remove_file(hwmon->device, &dev_attr_name);
hwmon_device_unregister(hwmon->device);
@@ -569,15 +620,30 @@ thermal_add_hwmon_sysfs(struct thermal_z
static void
thermal_remove_hwmon_sysfs(struct thermal_zone_device *tz)
{
- struct thermal_hwmon_device *hwmon = tz->hwmon;
+ struct thermal_hwmon_device *hwmon;
+ struct thermal_hwmon_temp *temp;

- tz->hwmon = NULL;
- device_remove_file(hwmon->device, &tz->temp_input.attr);
+ hwmon = thermal_hwmon_lookup_by_type(tz);
+ if (unlikely(!hwmon)) {
+ /* Should never happen... */
+ dev_dbg(&tz->device, "hwmon device lookup failed!\n");
+ return;
+ }
+
+ temp = thermal_hwmon_lookup_temp(hwmon, tz);
+ if (unlikely(!temp)) {
+ /* Should never happen... */
+ dev_dbg(&tz->device, "temperature input lookup failed!\n");
+ return;
+ }
+
+ device_remove_file(hwmon->device, &temp->temp_input.attr);
if (tz->ops->get_crit_temp)
- device_remove_file(hwmon->device, &tz->temp_crit.attr);
+ device_remove_file(hwmon->device, &temp->temp_crit.attr);

mutex_lock(&thermal_list_lock);
- list_del(&tz->hwmon_node);
+ list_del(&temp->hwmon_node);
+ kfree(temp);
if (!list_empty(&hwmon->tz_list)) {
mutex_unlock(&thermal_list_lock);
return;
--- linux-2.6.39-rc4.orig/include/linux/thermal.h 2011-04-25 16:16:10.000000000 +0200
+++ linux-2.6.39-rc4/include/linux/thermal.h 2011-04-26 10:40:46.000000000 +0200
@@ -85,22 +85,6 @@ struct thermal_cooling_device {
((long)t-2732+5)/10 : ((long)t-2732-5)/10)
#define CELSIUS_TO_KELVIN(t) ((t)*10+2732)

-#if defined(CONFIG_THERMAL_HWMON)
-/* thermal zone devices with the same type share one hwmon device */
-struct thermal_hwmon_device {
- char type[THERMAL_NAME_LENGTH];
- struct device *device;
- int count;
- struct list_head tz_list;
- struct list_head node;
-};
-
-struct thermal_hwmon_attr {
- struct device_attribute attr;
- char name[16];
-};
-#endif
-
struct thermal_zone_device {
int id;
char type[THERMAL_NAME_LENGTH];
@@ -120,12 +104,6 @@ struct thermal_zone_device {
struct mutex lock; /* protect cooling devices list */
struct list_head node;
struct delayed_work poll_queue;
-#if defined(CONFIG_THERMAL_HWMON)
- struct list_head hwmon_node;
- struct thermal_hwmon_device *hwmon;
- struct thermal_hwmon_attr temp_input; /* hwmon sys attr */
- struct thermal_hwmon_attr temp_crit; /* hwmon sys attr */
-#endif
};
/* Adding event notification support elements */
#define THERMAL_GENL_FAMILY_NAME "thermal_event"

--
Jean Delvare

2011-04-26 15:52:31

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

On Tue, Apr 26, 2011 at 11:04:07AM -0400, Jean Delvare wrote:
> THERMAL_HWMON is implemented inside the thermal_sys driver and has no
> effect on drivers implementing thermal zones, so they shouldn't see
> anything related to it in <linux/thermal.h>. Making the THERMAL_HWMON
> implementation fully internal has two advantages beyond the cleaner
> design:
> * This avoids rebuilding all thermal drivers if the THERMAL_HWMON
> implementation changes, or if CONFIG_THERMAL_HWMON gets enabled or
> disabled.
> * This avoids breaking the thermal kABI in these cases too, which
> should make distributions happy.
>
> The only drawback I can see is slightly higher memory fragmentation,
> as the number of kzalloc() calls will increase by one per thermal zone.
> But I doubt it will be a problem in practice, as I've never seen a
> system with more than two thermal zones.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Rene Herman <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Guenter Roeck <[email protected]>
> ---
> * If memory fragmentation is really a concern to anyone, it would be
> possible to save one kalloc for the first temperature input of each
> zone type, as the price of slightly more complex code.
>
> * Removal code path is untested, as I have never been able to unload
> the thermal_sys module on any of my systems. Something is pinning it
> and I have no idea what it is.
>
Doesn't lsmod show the culprit ?

Otherwise

Acked-by: Guenter Roeck <[email protected]>

2011-04-26 15:53:01

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] thermal: Hide CONFIG_THERMAL_HWMON

On Tue, Apr 26, 2011 at 11:02:08AM -0400, Jean Delvare wrote:
> It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311.
> Anybody running a kernel >= 2.6.40 would also be running a recent
> enough version of lm-sensors.
>
> Actually having CONFIG_THERMAL_HWMON is pretty convenient so instead
> of dopping it, we keep it but hide it.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Rene Herman <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Guenter Roeck <[email protected]>

Acked-by: Guenter Roeck <[email protected]>

2011-04-26 15:53:17

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 2/3] thermal: Split hwmon lookup to a separate function

On Tue, Apr 26, 2011 at 11:02:53AM -0400, Jean Delvare wrote:
> We'll soon need to reuse it.
>
> Signed-off-by: Jean Delvare <[email protected]>
> Cc: Rene Herman <[email protected]>
> Cc: Len Brown <[email protected]>
> Cc: Guenter Roeck <[email protected]>

Acked-by: Guenter Roeck <[email protected]>

2011-04-26 16:29:14

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

On Tue, 26 Apr 2011 08:52:12 -0700, Guenter Roeck wrote:
> On Tue, Apr 26, 2011 at 11:04:07AM -0400, Jean Delvare wrote:
> > THERMAL_HWMON is implemented inside the thermal_sys driver and has no
> > effect on drivers implementing thermal zones, so they shouldn't see
> > anything related to it in <linux/thermal.h>. Making the THERMAL_HWMON
> > implementation fully internal has two advantages beyond the cleaner
> > design:
> > * This avoids rebuilding all thermal drivers if the THERMAL_HWMON
> > implementation changes, or if CONFIG_THERMAL_HWMON gets enabled or
> > disabled.
> > * This avoids breaking the thermal kABI in these cases too, which
> > should make distributions happy.
> >
> > The only drawback I can see is slightly higher memory fragmentation,
> > as the number of kzalloc() calls will increase by one per thermal zone.
> > But I doubt it will be a problem in practice, as I've never seen a
> > system with more than two thermal zones.
> >
> > Signed-off-by: Jean Delvare <[email protected]>
> > Cc: Rene Herman <[email protected]>
> > Cc: Len Brown <[email protected]>
> > Cc: Guenter Roeck <[email protected]>
> > ---
> > * If memory fragmentation is really a concern to anyone, it would be
> > possible to save one kalloc for the first temperature input of each
> > zone type, as the price of slightly more complex code.
> >
> > * Removal code path is untested, as I have never been able to unload
> > the thermal_sys module on any of my systems. Something is pinning it
> > and I have no idea what it is.
> >
> Doesn't lsmod show the culprit ?

No, it's not a module dependency. The reference counter is set to 1,
so somewhere in the kernel something is taking a reference to the
module and won't release it. I wish this was better instrumented so
that it would be possible to know who is doing that.

> Otherwise
>
> Acked-by: Guenter Roeck <[email protected]>

Thanks for the reviews.

--
Jean Delvare

2011-04-26 17:43:54

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

On Tue, 2011-04-26 at 12:29 -0400, Jean Delvare wrote:
> On Tue, 26 Apr 2011 08:52:12 -0700, Guenter Roeck wrote:
> > On Tue, Apr 26, 2011 at 11:04:07AM -0400, Jean Delvare wrote:
> > > THERMAL_HWMON is implemented inside the thermal_sys driver and has no
> > > effect on drivers implementing thermal zones, so they shouldn't see
> > > anything related to it in <linux/thermal.h>. Making the THERMAL_HWMON
> > > implementation fully internal has two advantages beyond the cleaner
> > > design:
> > > * This avoids rebuilding all thermal drivers if the THERMAL_HWMON
> > > implementation changes, or if CONFIG_THERMAL_HWMON gets enabled or
> > > disabled.
> > > * This avoids breaking the thermal kABI in these cases too, which
> > > should make distributions happy.
> > >
> > > The only drawback I can see is slightly higher memory fragmentation,
> > > as the number of kzalloc() calls will increase by one per thermal zone.
> > > But I doubt it will be a problem in practice, as I've never seen a
> > > system with more than two thermal zones.
> > >
> > > Signed-off-by: Jean Delvare <[email protected]>
> > > Cc: Rene Herman <[email protected]>
> > > Cc: Len Brown <[email protected]>
> > > Cc: Guenter Roeck <[email protected]>
> > > ---
> > > * If memory fragmentation is really a concern to anyone, it would be
> > > possible to save one kalloc for the first temperature input of each
> > > zone type, as the price of slightly more complex code.
> > >
> > > * Removal code path is untested, as I have never been able to unload
> > > the thermal_sys module on any of my systems. Something is pinning it
> > > and I have no idea what it is.
> > >
> > Doesn't lsmod show the culprit ?
>
> No, it's not a module dependency. The reference counter is set to 1,
> so somewhere in the kernel something is taking a reference to the
> module and won't release it. I wish this was better instrumented so
> that it would be possible to know who is doing that.

The most likely culprit seems to be acpi.

Guenter

2011-04-26 19:40:14

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

Hi Guenter,

On Tue, 26 Apr 2011 10:43:32 -0700, Guenter Roeck wrote:
> On Tue, 2011-04-26 at 12:29 -0400, Jean Delvare wrote:
> > On Tue, 26 Apr 2011 08:52:12 -0700, Guenter Roeck wrote:
> > > On Tue, Apr 26, 2011 at 11:04:07AM -0400, Jean Delvare wrote:
> > > > * Removal code path is untested, as I have never been able to unload
> > > > the thermal_sys module on any of my systems. Something is pinning it
> > > > and I have no idea what it is.
> > > >
> > > Doesn't lsmod show the culprit ?
> >
> > No, it's not a module dependency. The reference counter is set to 1,

Sorry I realize I have been inaccurate. thermal_sys indeed depends on
the processor module, and that's what prevents me from unloading it.
It's the processor module which has a reference count of 1, and no
dependency, so I have no idea how I could unload it.

> > so somewhere in the kernel something is taking a reference to the
> > module and won't release it. I wish this was better instrumented so
> > that it would be possible to know who is doing that.
>
> The most likely culprit seems to be acpi.

I'm not sure. I don't see any relevant call to try_module_get under
drivers/acpi, and I'm not aware of any other way to increase the
reference count.

--
Jean Delvare

2011-04-26 21:00:52

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

On Tue, 2011-04-26 at 15:39 -0400, Jean Delvare wrote:
> Hi Guenter,
>
> On Tue, 26 Apr 2011 10:43:32 -0700, Guenter Roeck wrote:
> > On Tue, 2011-04-26 at 12:29 -0400, Jean Delvare wrote:
> > > On Tue, 26 Apr 2011 08:52:12 -0700, Guenter Roeck wrote:
> > > > On Tue, Apr 26, 2011 at 11:04:07AM -0400, Jean Delvare wrote:
> > > > > * Removal code path is untested, as I have never been able to unload
> > > > > the thermal_sys module on any of my systems. Something is pinning it
> > > > > and I have no idea what it is.
> > > > >
> > > > Doesn't lsmod show the culprit ?
> > >
> > > No, it's not a module dependency. The reference counter is set to 1,
>
> Sorry I realize I have been inaccurate. thermal_sys indeed depends on
> the processor module, and that's what prevents me from unloading it.
> It's the processor module which has a reference count of 1, and no
> dependency, so I have no idea how I could unload it.
>
You mean the ACPI processor driver ? This comment might explain it:

/*
* We keep the driver loaded even when ACPI is not running.
* This is needed for the powernow-k8 driver, that works even without
* ACPI, but needs symbols from this driver
*/

> > > so somewhere in the kernel something is taking a reference to the
> > > module and won't release it. I wish this was better instrumented so
> > > that it would be possible to know who is doing that.
> >
> > The most likely culprit seems to be acpi.
>
> I'm not sure. I don't see any relevant call to try_module_get under
> drivers/acpi, and I'm not aware of any other way to increase the
> reference count.
>
What happens if the calling code (such as, in my case here, the acpi
video code) gets built into the kernel ? Would that force the module to
be and remain loaded ?

Guenter

2011-04-27 11:59:36

by Jean Delvare

[permalink] [raw]
Subject: Re: [PATCH v2 3/3] thermal: Make THERMAL_HWMON implementation fully internal

Hi Guenter,

On Tue, 26 Apr 2011 14:00:31 -0700, Guenter Roeck wrote:
> On Tue, 2011-04-26 at 15:39 -0400, Jean Delvare wrote:
> > Sorry I realize I have been inaccurate. thermal_sys indeed depends on
> > the processor module, and that's what prevents me from unloading it.
> > It's the processor module which has a reference count of 1, and no
> > dependency, so I have no idea how I could unload it.
> >
> You mean the ACPI processor driver ? This comment might explain it:
>
> /*
> * We keep the driver loaded even when ACPI is not running.
> * This is needed for the powernow-k8 driver, that works even without
> * ACPI, but needs symbols from this driver
> */

I doubt it. I don't have powernow-k8 loaded on the affected systems.
And if I did, that would be a regular inter-module dependency that
would be listed by lsmod.

> > (...)
> > I'm not sure. I don't see any relevant call to try_module_get under
> > drivers/acpi, and I'm not aware of any other way to increase the
> > reference count.
> >
> What happens if the calling code (such as, in my case here, the acpi
> video code) gets built into the kernel ? Would that force the module to
> be and remain loaded ?

For regular inter-module dependencies, that's not even possible. You
can't build into the kernel a driver which depends on a symbol exported
by a driver which is built as a module. It will break at link time.

For reference counts increased by try_module_get(), it is possible, but
that doesn't necessarily make a difference from the all-modules case:
the reference count can be increased at any time, not only during
driver initialization, so the corresponding module_put doesn't
necessarily happen during module unloading (which indeed never happens
for a built-in driver.)

I tried booting in initlevel 1, but it didn't help, the reference count
of module processor is stuck to 1. So it's not user-space causing it...
it's something in the kernel itself. But I really don't have the time
to investigate this further, especially when I have no idea where to
look next.

--
Jean Delvare