On Tue, 15 Mar 2005 11:59:22 +0100, Giuseppe Bilotta said:
> > According to your patch, the C840 has 2 temp sensors. I'll have to figure
> > out what the second one is (prob either the GPU or the disk drive?)
>
> If it runs over 40 C easily it's probably the GPU :)

Well, (a) the next rev of the patch will hopefully provide more access to the
second thermal probe than just detecting its existence (it still doesn't do
the sysfs or whatever magic to make the actual value accessible), and (b) the
probe I *know* about is on the CPU, and runs over 40C easily as well (it's sitting
at 49C right now). Remember we're talking about a laptop here, there's not
a lot of room for a big heat sink in there.. ;)

Attachments:

(No filename) (226.00 B)

2005-03-15 22:36:42

by Frank Sorenson

[permalink] [raw]

Subject: Re: [PATCH 0/5] I8K driver facelift

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

[email protected] wrote:
> Well, (a) the next rev of the patch will hopefully provide more access to the
> second thermal probe than just detecting its existence (it still doesn't do
> the sysfs or whatever magic to make the actual value accessible), and (b) the
> probe I *know* about is on the CPU, and runs over 40C easily as well (it's sitting
> at 49C right now). Remember we're talking about a laptop here, there's not
> a lot of room for a big heat sink in there.. ;)

I've been trying to work out how to do this through dynamic sysfs
attributes, but I have not found a way to create arbitrary attributes
like this. It's not hard to define them at kernel compile time, but
selecting the right number of sensors to compile in seems arbitrary. My
Inspiron 9200 has 4 sensors, and who knows how many next year's model
will have. It just doesn't seem like the Linux Kernel way of doing
things to arbitrarily limit it like this.

I've looked into several ways of creating sysfs attributes, but haven't
found anything that works right/well. One of the most interesting was
in this past LKML thread - http://lkml.org/lkml/2004/8/20/287 If I
could replace the sysfs_attr_show() with my own, I believe that might
work (the attribute is passed into the function, so the name should be
available).

It's odd that it's so easy to compile sysfs attributes into the kernel,
but nobody seems to know how to generate them dynamically.

Thoughts? Suggestions?

Thanks,
Frank
- --
Frank Sorenson - KD7TZK
Systems Manager, Computer Science Department
Brigham Young University
[email protected]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.6 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFCN2LeaI0dwg4A47wRAhWEAKC+CcoLmoyvS6RXy7n7gtTnKjPXsACgtCbE
zofgMMEmc5mAzrQKdKwpIMQ=
=xNOU
-----END PGP SIGNATURE-----

2005-03-16 21:49:44

by Frank Sorenson

[permalink] [raw]

Subject: Re: [PATCH 0/5] I8K driver facelift

Okay, I replaced the sysfs_ops with ops of my own, and now all the show
and store functions also accept the name of the attribute as a parameter.
This lets the functions know what attribute is being accessed, and allows
us to create attributes that share show and store functions, so things
don't need to be defined at compile time (I feel slightly evil!).

This patch puts the correct number of temp sensors and fans into sysfs,
and only exposes power_status if enabled by the power_status module
parameter.

This patch applies on top of Dmitry Torokov's patches. Comments welcome!

Thanks,
Frank

Signed-off-by: Frank Sorenson <[email protected]>

--- 2.6.11-a/drivers/char/i8k.c 2005-03-12 18:47:55.000000000 -0700
+++ 2.6.11-b/drivers/char/i8k.c 2005-03-16 14:23:40.000000000 -0700
@@ -23,12 +23,14 @@
#include <linux/seq_file.h>
#include <linux/dmi.h>
#include <linux/device.h>
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
#include <asm/uaccess.h>
#include <asm/io.h>

#include <linux/i8k.h>

-#define I8K_VERSION "1.14 21/02/2005"
+#define I8K_VERSION "2005-03-16"

#define I8K_SMM_FN_STATUS 0x0025
#define I8K_SMM_POWER_STATUS 0x0069
@@ -36,7 +38,8 @@
#define I8K_SMM_GET_FAN 0x00a3
#define I8K_SMM_GET_SPEED 0x02a3
#define I8K_SMM_GET_TEMP 0x10a3
-#define I8K_SMM_GET_DELL_SIG 0xffa3
+#define I8K_SMM_GET_DELL_SIG1 0xfea3
+#define I8K_SMM_GET_DELL_SIG2 0xffa3
#define I8K_SMM_BIOS_VERSION 0x00a6

#define I8K_FAN_MULT 30
@@ -54,7 +57,12 @@

#define I8K_TEMPERATURE_BUG 1

+#define to_dev(d) container_of(d, struct device, kobj)
+#define to_dev_attr(_attr) container_of(_attr,struct device_attribute,attr)
+
static char bios_version[4];
+static int num_temps = 0;
+static int num_fans = 0;

MODULE_AUTHOR("Massimo Dal Zotto ([email protected])");
MODULE_DESCRIPTION("Driver for accessing SMM BIOS on Dell laptops");
@@ -80,6 +88,8 @@
static int i8k_ioctl(struct inode *, struct file *, unsigned int,
unsigned long);

+static struct kobj_type *backup_ktype;
+
static struct file_operations i8k_fops = {
.open = i8k_open_fs,
.read = seq_read,
@@ -94,7 +104,7 @@
};

static struct platform_device *i8k_device;
-
+
struct smm_regs {
unsigned int eax;
unsigned int ebx __attribute__ ((packed));
@@ -156,7 +166,7 @@
{
struct smm_regs regs = { .eax = I8K_SMM_BIOS_VERSION, };

- return i8k_smm(&regs) < 0 ? : regs.eax;
+ return i8k_smm(&regs) ? : regs.eax;
}

/*
@@ -201,10 +211,10 @@
*/
static int i8k_get_fan_status(int fan)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, .ebx = (fan & 0xff)};

regs.ebx = fan & 0xff;
- return i8k_smm(&regs) < 0 ? : regs.eax & 0xff;
+ return i8k_smm(&regs) ? : regs.eax & 0xff;
}

/*
@@ -215,7 +225,7 @@
struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };

regs.ebx = fan & 0xff;
- return i8k_smm(&regs) < 0 ? : (regs.eax & 0xffff) * I8K_FAN_MULT;
+ return i8k_smm(&regs) ? : (regs.eax & 0xffff) * I8K_FAN_MULT;
}

/*
@@ -228,15 +238,15 @@
speed = (speed < 0) ? 0 : ((speed > I8K_FAN_MAX) ? I8K_FAN_MAX : speed);
regs.ebx = (fan & 0xff) | (speed << 8);

- return i8k_smm(&regs) < 0 ? : i8k_get_fan_status(fan);
+ return i8k_smm(&regs) ? : i8k_get_fan_status(fan);
}

/*
* Read the cpu temperature.
*/
-static int i8k_get_cpu_temp(void)
+static int i8k_get_temp(int sensor)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, };
+ struct smm_regs regs = { .eax = I8K_SMM_GET_TEMP, .ebx = sensor };
int rc;
int temp;

@@ -268,9 +278,14 @@
return temp;
}

-static int i8k_get_dell_signature(void)
+static int i8k_get_cpu_temp(void)
{
- struct smm_regs regs = { .eax = I8K_SMM_GET_DELL_SIG, };
+ return i8k_get_temp(0);
+}
+
+static int i8k_get_dell_sig_aux(int fn)
+{
+ struct smm_regs regs = { .eax = fn, };
int rc;

if ((rc = i8k_smm(&regs)) < 0)
@@ -279,6 +294,17 @@
return regs.eax == 1145651527 && regs.edx == 1145392204 ? 0 : -1;
}

+static int i8k_get_dell_signature(void)
+{
+ int rc;
+
+ if (((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG1)) < 0) &&
+ ((rc=i8k_get_dell_sig_aux(I8K_SMM_GET_DELL_SIG2)) < 0)) {
+ return rc;
+ }
+ return 0;
+}
+
static int i8k_ioctl(struct inode *ip, struct file *fp, unsigned int cmd,
unsigned long arg)
{
@@ -414,87 +440,112 @@
return single_open(file, i8k_proc_show, NULL);
}

-static ssize_t i8k_sysfs_cpu_temp_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_cpu_temp_show(struct device *dev, char *buf,
+ const char *name)
{
int temp = i8k_get_cpu_temp();

return temp < 0 ? -EIO : sprintf(buf, "%d\n", temp);
}

-static ssize_t i8k_sysfs_fan1_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_temp_show(struct device *dev, char *buf,
+ const char *name)
{
- int status = i8k_get_fan_status(0);
- return status < 0 ? -EIO : sprintf(buf, "%d\n", status);
-}
-
-static ssize_t i8k_sysfs_fan1_set(struct device *dev, const char *buf, size_t count)
-{
- unsigned long state;
+ unsigned long temp_num;
+ int temp;
char *rest;

- if (restricted && !capable(CAP_SYS_ADMIN))
- return -EPERM;
-
- state = simple_strtoul(buf, &rest, 10);
- if (*rest || state > I8K_FAN_MAX)
- return -EINVAL;
+ temp_num = simple_strtoul(name + 4, &rest, 10);
+ temp = i8k_get_temp(temp_num - 1);

- if (i8k_set_fan(0, state) < 0)
- return -EIO;
-
- return count;
+ return temp < 0 ? -EIO : sprintf(buf, "%d\n", temp);
}

-static ssize_t i8k_sysfs_fan2_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_fan_show(struct device *dev, char *buf,
+ const char *name)
{
- int status = i8k_get_fan_status(1);
+ unsigned long fan_num;
+ int status;
+ char *rest;
+
+ fan_num = simple_strtoul(name + 3, &rest, 10);
+ status = i8k_get_fan_status(fan_num - 1);
return status < 0 ? -EIO : sprintf(buf, "%d\n", status);
}

-static ssize_t i8k_sysfs_fan2_set(struct device *dev, const char *buf, size_t count)
+static ssize_t i8k_sysfs_fan_set(struct device *dev, const char *buf,
+ size_t count, const char *name)
{
+ unsigned long fan_num;
unsigned long state;
char *rest;

if (restricted && !capable(CAP_SYS_ADMIN))
return -EPERM;

+ fan_num = simple_strtoul(name + 3, &rest, 10);
+
state = simple_strtoul(buf, &rest, 10);
if (*rest || state > I8K_FAN_MAX)
return -EINVAL;

- if (i8k_set_fan(1, state) < 0)
+ if (i8k_set_fan(fan_num - 1, state) < 0)
return -EIO;

return count;
}

-static ssize_t i8k_sysfs_fan1_speed_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_fan_speed_show(struct device *dev, char *buf,
+ const char *name)
{
- int speed = i8k_get_fan_speed(0);
- return speed < 0 ? -EIO : sprintf(buf, "%d\n", speed);
-}
+ unsigned long fan_num;
+ char *rest;
+ int speed;

-static ssize_t i8k_sysfs_fan2_speed_show(struct device *dev, char *buf)
-{
- int speed = i8k_get_fan_speed(1);
+ fan_num = simple_strtoul(name + 3, &rest, 10);
+ speed = i8k_get_fan_speed(fan_num - 1);
return speed < 0 ? -EIO : sprintf(buf, "%d\n", speed);
}

-static ssize_t i8k_sysfs_power_status_show(struct device *dev, char *buf)
+static ssize_t i8k_sysfs_power_status_show(struct device *dev, char *buf,
+ char *name)
{
int status = power_status ? i8k_get_power_status() : -1;
return status < 0 ? -EIO : sprintf(buf, "%d\n", status);
}

-static struct device_attribute i8k_device_attrs[] = {
- __ATTR(cpu_temp, 0444, i8k_sysfs_cpu_temp_show, NULL),
- __ATTR(fan1_state, 0644, i8k_sysfs_fan1_show, i8k_sysfs_fan1_set),
- __ATTR(fan2_state, 0644, i8k_sysfs_fan2_show, i8k_sysfs_fan2_set),
- __ATTR(fan1_speed, 0444, i8k_sysfs_fan1_speed_show, NULL),
- __ATTR(fan2_speed, 0444, i8k_sysfs_fan2_speed_show, NULL),
- __ATTR(power_status, 0444, i8k_sysfs_power_status_show, NULL),
- __ATTR_NULL
+static ssize_t i8k_sysfs_show(struct kobject *kobj, struct attribute *attr,
+ char * buf)
+{
+ struct device_attribute * dev_attr = to_dev_attr(attr);
+ struct device * dev = to_dev(kobj);
+ int (*func3)(struct device *, char *, char *);
+
+ if (dev_attr->show) {
+ func3 = dev_attr->show;
+ return (*func3)(dev, buf, dev_attr->attr.name);
+ }
+ return -EINVAL;
+}
+
+static ssize_t i8k_sysfs_store(struct kobject *kobj, struct attribute *attr,
+ const char *buf, size_t count)
+{
+ struct device_attribute * dev_attr = to_dev_attr(attr);
+ struct device * dev = to_dev(kobj);
+ int (*func4)(struct device *, const char *, size_t, char *);
+
+ if (dev_attr->store)
+ {
+ func4 = dev_attr->store;
+ return (*func4)(dev, buf, count, dev_attr->attr.name);
+ }
+ return -EINVAL;
+}
+
+static struct sysfs_ops i8k_sysfs_ops = {
+ .show = i8k_sysfs_show,
+ .store = i8k_sysfs_store,
};

static struct dmi_system_id __initdata i8k_dmi_table[] = {
@@ -506,6 +557,13 @@
},
},
{
+ .ident = "Dell Inspiron",
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Inspiron"),
+ },
+ },
+ {
.ident = "Dell Latitude",
.matches = {
DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer"),
@@ -578,6 +636,105 @@
return 0;
}

+static int sysfs_add_generic(char *dev_name, int mode, void *show, void *store)
+{
+ char *name;
+ struct device_attribute *attr;
+ int err;
+
+ name = kmalloc(strlen(dev_name) + 1, GFP_KERNEL);
+ if (name == NULL)
+ return -ENOMEM;
+ strncpy(name, dev_name, strlen(dev_name));
+ name[strlen(dev_name)] = '\0';
+ attr = kmalloc(sizeof *attr, GFP_KERNEL);
+ if (attr == NULL)
+ return -ENOMEM;
+ *attr = ((struct device_attribute) {
+ .attr = {
+ .name = name,
+ .mode = mode,
+ },
+ .show = show,
+ .store = store,
+ });
+ err = device_create_file(&i8k_device->dev, attr);
+ if (err)
+ {
+ device_remove_file(&i8k_device->dev, attr);
+ kfree(attr);
+ kfree(name);
+ return -1;
+ }
+ return 0;
+}
+
+static int sysfs_add_temp(int temp_num)
+{
+ char temp_name[30];
+
+ sprintf(temp_name, "temp%d", temp_num + 1);
+ return sysfs_add_generic(temp_name, 0444, i8k_sysfs_temp_show, NULL);
+}
+
+static int sysfs_add_fan(int fan_num)
+{
+ char fan_state_name[30];
+ char fan_speed_name[30];
+
+ sprintf(fan_state_name, "fan%d_state", fan_num + 1);
+ sprintf(fan_speed_name, "fan%d_speed", fan_num + 1);
+
+ sysfs_add_generic(fan_state_name, 0644,
+ i8k_sysfs_fan_show, i8k_sysfs_fan_set);
+ sysfs_add_generic(fan_speed_name, 0444,
+ i8k_sysfs_fan_speed_show, NULL);
+ return 0;
+}
+
+static int i8k_redirect_sysfs_ops(void)
+{
+ struct device *dev;
+ struct kobject *kobj;
+ struct kobj_type *ktype;
+ struct kobj_type *temp_ktype;
+ struct kset *kset;
+
+ dev = &i8k_device->dev;
+ kobj = &dev->kobj;
+ kset = kobj->kset;
+ ktype = kset->ktype;
+
+ temp_ktype = kmalloc(sizeof *temp_ktype, GFP_KERNEL);
+
+ temp_ktype->release = ktype->release;
+ temp_ktype->sysfs_ops = &i8k_sysfs_ops;
+ temp_ktype->default_attrs = ktype->default_attrs;
+
+ backup_ktype = ktype;
+ kset->ktype = temp_ktype;
+
+ return 0;
+}
+
+static int i8k_restore_sysfs_ops(void)
+{
+ struct device *dev;
+ struct kobject *kobj;
+ struct kset *kset;
+ struct kobj_type *temp_ktype;
+
+ dev = &i8k_device->dev;
+ kobj = &dev->kobj;
+ kset = kobj->kset;
+ temp_ktype = kset->ktype;
+
+ kset->ktype = backup_ktype;
+ kfree(temp_ktype);
+
+ return 0;
+}
+
static int __init i8k_init(void)
{
struct proc_dir_entry *proc_i8k;
@@ -587,6 +744,11 @@
if (i8k_probe())
return -ENODEV;

+ while (i8k_get_temp(num_temps) >= 0) num_temps ++;
+ printk(KERN_INFO "i8k: found %d temperature sensors\n", num_temps);
+ while (i8k_get_fan_status(num_fans) >= 0) num_fans ++;
+ printk(KERN_INFO "i8k: found %d fans\n", num_fans);
+
/* Register the proc entry */
proc_i8k = create_proc_entry("i8k", 0, NULL);
if (!proc_i8k)
@@ -605,21 +767,31 @@
goto fail2;
}

- for (i = 0; attr_name(i8k_device_attrs[i]); i++) {
- err = device_create_file(&i8k_device->dev, &i8k_device_attrs[i]);
+ i8k_redirect_sysfs_ops();
+
+ /* register the temp sensors */
+ for (i = 0; i < num_temps; i++) {
+ err = sysfs_add_temp(i);
+ if (err)
+ goto fail2;
+ }
+
+ /* register the fans */
+ for (i = 0; i < num_fans; i++) {
+ err = sysfs_add_fan(i);
if (err)
- goto fail3;
+ goto fail2;
}
+ sysfs_add_generic("cpu_temp", 0444, i8k_sysfs_cpu_temp_show, NULL);
+ if (power_status)
+ sysfs_add_generic("power_status", 0444,
+ i8k_sysfs_power_status_show, NULL);

printk(KERN_INFO
"Dell laptop SMM driver v%s Massimo Dal Zotto ([email protected])\n",
I8K_VERSION);

return 0;
-
-fail3: while (--i >= 0)
- device_remove_file(&i8k_device->dev, &i8k_device_attrs[i]);
- platform_device_unregister(i8k_device);
fail2: driver_unregister(&i8k_driver);
fail1: remove_proc_entry("i8k", NULL);
return err;
@@ -627,6 +799,7 @@

static void __exit i8k_exit(void)
{
+ i8k_restore_sysfs_ops();
platform_device_unregister(i8k_device);
driver_unregister(&i8k_driver);
remove_proc_entry("i8k", NULL);

2005-03-17 06:43:21

2005-03-17 14:49:56

On Wed, Apr 13, 2005 at 01:33:31AM -0500, Dmitry Torokhov wrote:
> On Thursday 24 March 2005 02:24, Greg KH wrote:
> > On Thu, Mar 17, 2005 at 03:16:24AM -0500, [email protected] wrote:
> > > On Wed, 16 Mar 2005 14:38:50 MST, Frank Sorenson said:
> > > > Okay, I replaced the sysfs_ops with ops of my own, and now all the show
> > > > and store functions also accept the name of the attribute as a parameter.
> > > > This lets the functions know what attribute is being accessed, and allows
> > > > us to create attributes that share show and store functions, so things
> > > > don't need to be defined at compile time (I feel slightly evil!).
> > > >
> > > > This patch puts the correct number of temp sensors and fans into sysfs,
> > > > and only exposes power_status if enabled by the power_status module
> > > > parameter.
> > >
> > > Works for me:
> > >
> > > [/sys/bus/platform/drivers/i8k/i8k]2 ls -l
> > > total 0
> > > lrwxrwxrwx 1 root root 0 Mar 17 03:02 bus -> ../../../bus/platform
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 cpu_temp
> > > -rw-r--r-- 1 root root 4096 Mar 17 03:01 detach_state
> > > lrwxrwxrwx 1 root root 0 Mar 17 03:02 driver -> ../../../bus/platform/drivers/i8k
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan1_speed
> > > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan1_state
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 fan2_speed
> > > -rw-r--r-- 1 root root 4096 Mar 17 03:02 fan2_state
> > > drwxr-xr-x 2 root root 0 Mar 17 03:02 power
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 power_status
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp1
> > > -r--r--r-- 1 root root 4096 Mar 17 03:02 temp2
> > >
> > > The valyes of the fan* settings, and cpu_temp match what's reported in /proc/i8k.
> >
> > Please match the same units and filename as the other i2c sensors. See
> > the documentation in the Documentation/i2c/ directory for what that
> > standard is, so userspace programs will "just work" with your devices.
> >
>
> Greg,
>
> I almost started doing what you just said but then I realized that none of
> the programs will "just work" because all of them will look into /sys/bus/i2c
> instead of /sys/bus/platform/i8k.
>
> For userspace tools to work transparently we would need something like
> /sys/class/sensor/{fan|temp|current}, but it is something I am not ready to do
> now - I need to finish input layer first.

The sensors developers are creating just such a class, see their hwmon
patch in their email archive if you are curious.

> So given above I think having private scheme for now is ok. Sooo... Can I get
> my attributes goups patch in so I can use it in i8k, please?

Ick, no, use the upcoming hwmon stuff instead :)

thanks,

greg k-h