| Message ID | 20181208005603.11721-1-michelesr@autistici.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dell-smm-hwmon.c: Additional temperature sensors | expand |
On 12/7/18 4:56 PM, Michele Sorcinelli wrote: > The code has been extended to support up to 10 temperature sensors. > > The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced > with i8k_get_temp() as in some laptop firmwares the related SMM procedure > is not implemented correctly so i8k_get_temp_type() can't be used to > discover temperature sensors. > > Signed-off-by: Michele Sorcinelli <michelesr@autistici.org> Please _always_ version your patches, and add a changelog, at least if you would like to see it applied. I have now two patches which look almost the same, meaning I would have to pull both versions, compare, and hope to apply the correct one. Anyway, I would like to get some feedback if this can cause regressions on systems which don't support that many sensors and maybe report something completely different if one tries to read the high-numbered sensors. I seem to recall that there was a reason for checking the type and not just trying to read sensor values. Guenter > --- > drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------ > 1 file changed, 94 insertions(+), 19 deletions(-) > mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c > old mode 100644 > new mode 100755 > index 367a8a617..b58f756ea > --- a/drivers/hwmon/dell-smm-hwmon.c > +++ b/drivers/hwmon/dell-smm-hwmon.c > @@ -82,9 +82,16 @@ static bool disallow_fan_support; > #define I8K_HWMON_HAVE_TEMP2 (1 << 1) > #define I8K_HWMON_HAVE_TEMP3 (1 << 2) > #define I8K_HWMON_HAVE_TEMP4 (1 << 3) > -#define I8K_HWMON_HAVE_FAN1 (1 << 4) > -#define I8K_HWMON_HAVE_FAN2 (1 << 5) > -#define I8K_HWMON_HAVE_FAN3 (1 << 6) > +#define I8K_HWMON_HAVE_TEMP5 (1 << 4) > +#define I8K_HWMON_HAVE_TEMP6 (1 << 5) > +#define I8K_HWMON_HAVE_TEMP7 (1 << 6) > +#define I8K_HWMON_HAVE_TEMP8 (1 << 7) > +#define I8K_HWMON_HAVE_TEMP9 (1 << 8) > +#define I8K_HWMON_HAVE_TEMP10 (1 << 9) > + > +#define I8K_HWMON_HAVE_FAN1 (1 << 10) > +#define I8K_HWMON_HAVE_FAN2 (1 << 11) > +#define I8K_HWMON_HAVE_FAN3 (1 << 12) > > MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)"); > MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>"); > @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3); > static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > 3); > +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4); > +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > + 4); > +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5); > +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > + 5); > +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6); > +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > + 6); > +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7); > +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > + 7); > +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8); > +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > + 8); > +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9); > +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, > + 9); > + > static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL, 0); > static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL, > 0); > @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = { > &sensor_dev_attr_temp3_label.dev_attr.attr, /* 5 */ > &sensor_dev_attr_temp4_input.dev_attr.attr, /* 6 */ > &sensor_dev_attr_temp4_label.dev_attr.attr, /* 7 */ > - &sensor_dev_attr_fan1_input.dev_attr.attr, /* 8 */ > - &sensor_dev_attr_fan1_label.dev_attr.attr, /* 9 */ > - &sensor_dev_attr_pwm1.dev_attr.attr, /* 10 */ > - &sensor_dev_attr_fan2_input.dev_attr.attr, /* 11 */ > - &sensor_dev_attr_fan2_label.dev_attr.attr, /* 12 */ > - &sensor_dev_attr_pwm2.dev_attr.attr, /* 13 */ > - &sensor_dev_attr_fan3_input.dev_attr.attr, /* 14 */ > - &sensor_dev_attr_fan3_label.dev_attr.attr, /* 15 */ > - &sensor_dev_attr_pwm3.dev_attr.attr, /* 16 */ > + &sensor_dev_attr_temp5_input.dev_attr.attr, /* 8 */ > + &sensor_dev_attr_temp5_label.dev_attr.attr, /* 9 */ > + &sensor_dev_attr_temp6_input.dev_attr.attr, /* 10 */ > + &sensor_dev_attr_temp6_label.dev_attr.attr, /* 11 */ > + &sensor_dev_attr_temp7_input.dev_attr.attr, /* 12 */ > + &sensor_dev_attr_temp7_label.dev_attr.attr, /* 13 */ > + &sensor_dev_attr_temp8_input.dev_attr.attr, /* 14 */ > + &sensor_dev_attr_temp8_label.dev_attr.attr, /* 15 */ > + &sensor_dev_attr_temp9_input.dev_attr.attr, /* 16 */ > + &sensor_dev_attr_temp9_label.dev_attr.attr, /* 17 */ > + &sensor_dev_attr_temp10_input.dev_attr.attr, /* 18 */ > + &sensor_dev_attr_temp10_label.dev_attr.attr, /* 19 */ > + &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */ > + &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */ > + &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */ > + &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */ > + &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */ > + &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */ > + &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */ > + &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */ > + &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */ > NULL > }; > > @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr, > if (index >= 6 && index <= 7 && > !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4)) > return 0; > - if (index >= 8 && index <= 10 && > + if (index >= 8 && index <= 9 && > + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5)) > + return 0; > + if (index >= 10 && index <= 11 && > + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6)) > + return 0; > + if (index >= 12 && index <= 13 && > + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7)) > + return 0; > + if (index >= 14 && index <= 15 && > + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8)) > + return 0; > + if (index >= 16 && index <= 17 && > + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9)) > + return 0; > + if (index >= 18 && index <= 19 && > + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10)) > + return 0; > + > + if (index >= 20 && index <= 22 && > !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1)) > return 0; > - if (index >= 11 && index <= 13 && > + if (index >= 23 && index <= 25 && > !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2)) > return 0; > - if (index >= 14 && index <= 16 && > + if (index >= 26 && index <= 28 && > !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3)) > return 0; > > @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void) > i8k_hwmon_flags = 0; > > /* CPU temperature attributes, if temperature type is OK */ > - err = i8k_get_temp_type(0); > + err = i8k_get_temp(0); > if (err >= 0) > i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1; > /* check for additional temperature sensors */ > - err = i8k_get_temp_type(1); > + err = i8k_get_temp(1); > if (err >= 0) > i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2; > - err = i8k_get_temp_type(2); > + err = i8k_get_temp(2); > if (err >= 0) > i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3; > - err = i8k_get_temp_type(3); > + err = i8k_get_temp(3); > if (err >= 0) > i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4; > + err = i8k_get_temp(4); > + if (err >= 0) > + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5; > + err = i8k_get_temp(5); > + if (err >= 0) > + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6; > + err = i8k_get_temp(6); > + if (err >= 0) > + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7; > + err = i8k_get_temp(7); > + if (err >= 0) > + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8; > + err = i8k_get_temp(8); > + if (err >= 0) > + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9; > + err = i8k_get_temp(9); > + if (err >= 0) > + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10; > > /* First fan attributes, if fan status or type is OK */ > err = i8k_get_fan_status(0); >
> Please _always_ version your patches, and add a changelog, at least if you > would like to see it applied. I have now two patches which look almost the same, > meaning I would have to pull both versions, compare, and hope to apply the > correct one. The only change is in i8k_is_visible(): // first version + if (index >= 20 && index <= 22 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1)) return 0; - if (index >= 11 && index <= 13 && + if (index >= 23 && index <= 25 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2)) return 0; - if (index >= 14 && index <= 16 && + if (index >= 26 && index <= 28 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3)) return 0; // second version + if (index >= 20 && index <= 22 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1)) return 0; - if (index >= 11 && index <= 13 && + if (index >= 23 && index <= 25 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2)) return 0; - if (index >= 14 && index <= 16 && + if (index >= 26 && index <= 28 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3)) return 0; Now the indexes in the if conditions have been changed to properly match the entries in the i8k_attrs array: &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */ &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */ &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */ &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */ &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */ &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */ &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */ &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */ &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */ > Anyway, I would like to get some feedback if this can cause regressions > on systems which don't support that many sensors and maybe report something > completely different if one tries to read the high-numbered sensors. > I seem to recall that there was a reason for checking the type and not just > trying to read sensor values. AFAIK, i8k_get_temp() should return error if the sensor is not available, thus the sensor won't be initialized in the hwmon interface. Unfortunately, I don't have other machines to test it. On 12/8/18 4:24 AM, Guenter Roeck wrote: > On 12/7/18 4:56 PM, Michele Sorcinelli wrote: >> The code has been extended to support up to 10 temperature sensors. >> >> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced >> with i8k_get_temp() as in some laptop firmwares the related SMM procedure >> is not implemented correctly so i8k_get_temp_type() can't be used to >> discover temperature sensors. >> >> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org> > > Please _always_ version your patches, and add a changelog, at least if you > would like to see it applied. I have now two patches which look almost the same, > meaning I would have to pull both versions, compare, and hope to apply the > correct one. > > Anyway, I would like to get some feedback if this can cause regressions > on systems which don't support that many sensors and maybe report something > completely different if one tries to read the high-numbered sensors. > I seem to recall that there was a reason for checking the type and not just > trying to read sensor values. > > Guenter > >> --- >> drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------ >> 1 file changed, 94 insertions(+), 19 deletions(-) >> mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c >> >> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c >> old mode 100644 >> new mode 100755 >> index 367a8a617..b58f756ea >> --- a/drivers/hwmon/dell-smm-hwmon.c >> +++ b/drivers/hwmon/dell-smm-hwmon.c >> @@ -82,9 +82,16 @@ static bool disallow_fan_support; >> #define I8K_HWMON_HAVE_TEMP2 (1 << 1) >> #define I8K_HWMON_HAVE_TEMP3 (1 << 2) >> #define I8K_HWMON_HAVE_TEMP4 (1 << 3) >> -#define I8K_HWMON_HAVE_FAN1 (1 << 4) >> -#define I8K_HWMON_HAVE_FAN2 (1 << 5) >> -#define I8K_HWMON_HAVE_FAN3 (1 << 6) >> +#define I8K_HWMON_HAVE_TEMP5 (1 << 4) >> +#define I8K_HWMON_HAVE_TEMP6 (1 << 5) >> +#define I8K_HWMON_HAVE_TEMP7 (1 << 6) >> +#define I8K_HWMON_HAVE_TEMP8 (1 << 7) >> +#define I8K_HWMON_HAVE_TEMP9 (1 << 8) >> +#define I8K_HWMON_HAVE_TEMP10 (1 << 9) >> + >> +#define I8K_HWMON_HAVE_FAN1 (1 << 10) >> +#define I8K_HWMON_HAVE_FAN2 (1 << 11) >> +#define I8K_HWMON_HAVE_FAN3 (1 << 12) >> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)"); >> MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>"); >> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3); >> static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> 3); >> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4); >> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> + 4); >> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5); >> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> + 5); >> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6); >> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> + 6); >> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7); >> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> + 7); >> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8); >> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> + 8); >> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9); >> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, >> + 9); >> + >> static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL, 0); >> static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL, >> 0); >> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = { >> &sensor_dev_attr_temp3_label.dev_attr.attr, /* 5 */ >> &sensor_dev_attr_temp4_input.dev_attr.attr, /* 6 */ >> &sensor_dev_attr_temp4_label.dev_attr.attr, /* 7 */ >> - &sensor_dev_attr_fan1_input.dev_attr.attr, /* 8 */ >> - &sensor_dev_attr_fan1_label.dev_attr.attr, /* 9 */ >> - &sensor_dev_attr_pwm1.dev_attr.attr, /* 10 */ >> - &sensor_dev_attr_fan2_input.dev_attr.attr, /* 11 */ >> - &sensor_dev_attr_fan2_label.dev_attr.attr, /* 12 */ >> - &sensor_dev_attr_pwm2.dev_attr.attr, /* 13 */ >> - &sensor_dev_attr_fan3_input.dev_attr.attr, /* 14 */ >> - &sensor_dev_attr_fan3_label.dev_attr.attr, /* 15 */ >> - &sensor_dev_attr_pwm3.dev_attr.attr, /* 16 */ >> + &sensor_dev_attr_temp5_input.dev_attr.attr, /* 8 */ >> + &sensor_dev_attr_temp5_label.dev_attr.attr, /* 9 */ >> + &sensor_dev_attr_temp6_input.dev_attr.attr, /* 10 */ >> + &sensor_dev_attr_temp6_label.dev_attr.attr, /* 11 */ >> + &sensor_dev_attr_temp7_input.dev_attr.attr, /* 12 */ >> + &sensor_dev_attr_temp7_label.dev_attr.attr, /* 13 */ >> + &sensor_dev_attr_temp8_input.dev_attr.attr, /* 14 */ >> + &sensor_dev_attr_temp8_label.dev_attr.attr, /* 15 */ >> + &sensor_dev_attr_temp9_input.dev_attr.attr, /* 16 */ >> + &sensor_dev_attr_temp9_label.dev_attr.attr, /* 17 */ >> + &sensor_dev_attr_temp10_input.dev_attr.attr, /* 18 */ >> + &sensor_dev_attr_temp10_label.dev_attr.attr, /* 19 */ >> + &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */ >> + &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */ >> + &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */ >> + &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */ >> + &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */ >> + &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */ >> + &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */ >> + &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */ >> + &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */ >> NULL >> }; >> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr, >> if (index >= 6 && index <= 7 && >> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4)) >> return 0; >> - if (index >= 8 && index <= 10 && >> + if (index >= 8 && index <= 9 && >> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5)) >> + return 0; >> + if (index >= 10 && index <= 11 && >> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6)) >> + return 0; >> + if (index >= 12 && index <= 13 && >> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7)) >> + return 0; >> + if (index >= 14 && index <= 15 && >> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8)) >> + return 0; >> + if (index >= 16 && index <= 17 && >> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9)) >> + return 0; >> + if (index >= 18 && index <= 19 && >> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10)) >> + return 0; >> + >> + if (index >= 20 && index <= 22 && >> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1)) >> return 0; >> - if (index >= 11 && index <= 13 && >> + if (index >= 23 && index <= 25 && >> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2)) >> return 0; >> - if (index >= 14 && index <= 16 && >> + if (index >= 26 && index <= 28 && >> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3)) >> return 0; >> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void) >> i8k_hwmon_flags = 0; >> /* CPU temperature attributes, if temperature type is OK */ >> - err = i8k_get_temp_type(0); >> + err = i8k_get_temp(0); >> if (err >= 0) >> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1; >> /* check for additional temperature sensors */ >> - err = i8k_get_temp_type(1); >> + err = i8k_get_temp(1); >> if (err >= 0) >> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2; >> - err = i8k_get_temp_type(2); >> + err = i8k_get_temp(2); >> if (err >= 0) >> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3; >> - err = i8k_get_temp_type(3); >> + err = i8k_get_temp(3); >> if (err >= 0) >> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4; >> + err = i8k_get_temp(4); >> + if (err >= 0) >> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5; >> + err = i8k_get_temp(5); >> + if (err >= 0) >> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6; >> + err = i8k_get_temp(6); >> + if (err >= 0) >> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7; >> + err = i8k_get_temp(7); >> + if (err >= 0) >> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8; >> + err = i8k_get_temp(8); >> + if (err >= 0) >> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9; >> + err = i8k_get_temp(9); >> + if (err >= 0) >> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10; >> /* First fan attributes, if fan status or type is OK */ >> err = i8k_get_fan_status(0); >> >
I'm sorry, the last diff I sent was wrong, this is the correct one.
Original version (wrong indexes):
+ if (index >= 20 && index <= 21 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
return 0;
- if (index >= 11 && index <= 13 &&
+ if (index >= 23 && index <= 24 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
return 0;
- if (index >= 14 && index <= 16 &&
+ if (index >= 26 && index <= 27 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
return 0;
Amended version (correct indexes):
+ if (index >= 20 && index <= 22 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
return 0;
- if (index >= 11 && index <= 13 &&
+ if (index >= 23 && index <= 25 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
return 0;
- if (index >= 14 && index <= 16 &&
+ if (index >= 26 && index <= 28 &&
!(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
return 0;
Again sorry for this, I appreciate that is very confusing.
On 12/8/18 3:34 PM, Michele Sorcinelli wrote:
>> Please _always_ version your patches, and add a changelog, at least if you
>> would like to see it applied. I have now two patches which look almost the same,
>> meaning I would have to pull both versions, compare, and hope to apply the
>> correct one.
>
> The only change is in i8k_is_visible():
>
> // first version
> + if (index >= 20 && index <= 22 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
> return 0;
> - if (index >= 11 && index <= 13 &&
> + if (index >= 23 && index <= 25 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
> return 0;
> - if (index >= 14 && index <= 16 &&
> + if (index >= 26 && index <= 28 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
> return 0;
>
> // second version
> + if (index >= 20 && index <= 22 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
> return 0;
> - if (index >= 11 && index <= 13 &&
> + if (index >= 23 && index <= 25 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
> return 0;
> - if (index >= 14 && index <= 16 &&
> + if (index >= 26 && index <= 28 &&
> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
> return 0;
>
> Now the indexes in the if conditions have been changed to properly match the
> entries in the i8k_attrs array:
>
> &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */
> &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */
> &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */
> &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */
> &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */
> &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */
> &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */
> &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */
> &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */
>
>> Anyway, I would like to get some feedback if this can cause regressions
>> on systems which don't support that many sensors and maybe report something
>> completely different if one tries to read the high-numbered sensors.
>> I seem to recall that there was a reason for checking the type and not just
>> trying to read sensor values.
>
> AFAIK, i8k_get_temp() should return error if the sensor is not available, thus
> the sensor won't be initialized in the hwmon interface. Unfortunately, I don't
> have other machines to test it.
>
>
> On 12/8/18 4:24 AM, Guenter Roeck wrote:
>> On 12/7/18 4:56 PM, Michele Sorcinelli wrote:
>>> The code has been extended to support up to 10 temperature sensors.
>>>
>>> The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced
>>> with i8k_get_temp() as in some laptop firmwares the related SMM procedure
>>> is not implemented correctly so i8k_get_temp_type() can't be used to
>>> discover temperature sensors.
>>>
>>> Signed-off-by: Michele Sorcinelli <michelesr@autistici.org>
>>
>> Please _always_ version your patches, and add a changelog, at least if you
>> would like to see it applied. I have now two patches which look almost the same,
>> meaning I would have to pull both versions, compare, and hope to apply the
>> correct one.
>>
>> Anyway, I would like to get some feedback if this can cause regressions
>> on systems which don't support that many sensors and maybe report something
>> completely different if one tries to read the high-numbered sensors.
>> I seem to recall that there was a reason for checking the type and not just
>> trying to read sensor values.
>>
>> Guenter
>>
>>> ---
>>> drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------
>>> 1 file changed, 94 insertions(+), 19 deletions(-)
>>> mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>>> old mode 100644
>>> new mode 100755
>>> index 367a8a617..b58f756ea
>>> --- a/drivers/hwmon/dell-smm-hwmon.c
>>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>>> @@ -82,9 +82,16 @@ static bool disallow_fan_support;
>>> #define I8K_HWMON_HAVE_TEMP2 (1 << 1)
>>> #define I8K_HWMON_HAVE_TEMP3 (1 << 2)
>>> #define I8K_HWMON_HAVE_TEMP4 (1 << 3)
>>> -#define I8K_HWMON_HAVE_FAN1 (1 << 4)
>>> -#define I8K_HWMON_HAVE_FAN2 (1 << 5)
>>> -#define I8K_HWMON_HAVE_FAN3 (1 << 6)
>>> +#define I8K_HWMON_HAVE_TEMP5 (1 << 4)
>>> +#define I8K_HWMON_HAVE_TEMP6 (1 << 5)
>>> +#define I8K_HWMON_HAVE_TEMP7 (1 << 6)
>>> +#define I8K_HWMON_HAVE_TEMP8 (1 << 7)
>>> +#define I8K_HWMON_HAVE_TEMP9 (1 << 8)
>>> +#define I8K_HWMON_HAVE_TEMP10 (1 << 9)
>>> +
>>> +#define I8K_HWMON_HAVE_FAN1 (1 << 10)
>>> +#define I8K_HWMON_HAVE_FAN2 (1 << 11)
>>> +#define I8K_HWMON_HAVE_FAN3 (1 << 12)
>>> MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)");
>>> MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
>>> @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3);
>>> static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> 3);
>>> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4);
>>> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> + 4);
>>> +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5);
>>> +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> + 5);
>>> +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6);
>>> +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> + 6);
>>> +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7);
>>> +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> + 7);
>>> +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8);
>>> +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> + 8);
>>> +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9);
>>> +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL,
>>> + 9);
>>> +
>>> static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL, 0);
>>> static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL,
>>> 0);
>>> @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = {
>>> &sensor_dev_attr_temp3_label.dev_attr.attr, /* 5 */
>>> &sensor_dev_attr_temp4_input.dev_attr.attr, /* 6 */
>>> &sensor_dev_attr_temp4_label.dev_attr.attr, /* 7 */
>>> - &sensor_dev_attr_fan1_input.dev_attr.attr, /* 8 */
>>> - &sensor_dev_attr_fan1_label.dev_attr.attr, /* 9 */
>>> - &sensor_dev_attr_pwm1.dev_attr.attr, /* 10 */
>>> - &sensor_dev_attr_fan2_input.dev_attr.attr, /* 11 */
>>> - &sensor_dev_attr_fan2_label.dev_attr.attr, /* 12 */
>>> - &sensor_dev_attr_pwm2.dev_attr.attr, /* 13 */
>>> - &sensor_dev_attr_fan3_input.dev_attr.attr, /* 14 */
>>> - &sensor_dev_attr_fan3_label.dev_attr.attr, /* 15 */
>>> - &sensor_dev_attr_pwm3.dev_attr.attr, /* 16 */
>>> + &sensor_dev_attr_temp5_input.dev_attr.attr, /* 8 */
>>> + &sensor_dev_attr_temp5_label.dev_attr.attr, /* 9 */
>>> + &sensor_dev_attr_temp6_input.dev_attr.attr, /* 10 */
>>> + &sensor_dev_attr_temp6_label.dev_attr.attr, /* 11 */
>>> + &sensor_dev_attr_temp7_input.dev_attr.attr, /* 12 */
>>> + &sensor_dev_attr_temp7_label.dev_attr.attr, /* 13 */
>>> + &sensor_dev_attr_temp8_input.dev_attr.attr, /* 14 */
>>> + &sensor_dev_attr_temp8_label.dev_attr.attr, /* 15 */
>>> + &sensor_dev_attr_temp9_input.dev_attr.attr, /* 16 */
>>> + &sensor_dev_attr_temp9_label.dev_attr.attr, /* 17 */
>>> + &sensor_dev_attr_temp10_input.dev_attr.attr, /* 18 */
>>> + &sensor_dev_attr_temp10_label.dev_attr.attr, /* 19 */
>>> + &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */
>>> + &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */
>>> + &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */
>>> + &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */
>>> + &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */
>>> + &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */
>>> + &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */
>>> + &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */
>>> + &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */
>>> NULL
>>> };
>>> @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
>>> if (index >= 6 && index <= 7 &&
>>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4))
>>> return 0;
>>> - if (index >= 8 && index <= 10 &&
>>> + if (index >= 8 && index <= 9 &&
>>> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5))
>>> + return 0;
>>> + if (index >= 10 && index <= 11 &&
>>> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6))
>>> + return 0;
>>> + if (index >= 12 && index <= 13 &&
>>> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7))
>>> + return 0;
>>> + if (index >= 14 && index <= 15 &&
>>> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8))
>>> + return 0;
>>> + if (index >= 16 && index <= 17 &&
>>> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9))
>>> + return 0;
>>> + if (index >= 18 && index <= 19 &&
>>> + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10))
>>> + return 0;
>>> +
>>> + if (index >= 20 && index <= 22 &&
>>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1))
>>> return 0;
>>> - if (index >= 11 && index <= 13 &&
>>> + if (index >= 23 && index <= 25 &&
>>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2))
>>> return 0;
>>> - if (index >= 14 && index <= 16 &&
>>> + if (index >= 26 && index <= 28 &&
>>> !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3))
>>> return 0;
>>> @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void)
>>> i8k_hwmon_flags = 0;
>>> /* CPU temperature attributes, if temperature type is OK */
>>> - err = i8k_get_temp_type(0);
>>> + err = i8k_get_temp(0);
>>> if (err >= 0)
>>> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1;
>>> /* check for additional temperature sensors */
>>> - err = i8k_get_temp_type(1);
>>> + err = i8k_get_temp(1);
>>> if (err >= 0)
>>> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2;
>>> - err = i8k_get_temp_type(2);
>>> + err = i8k_get_temp(2);
>>> if (err >= 0)
>>> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3;
>>> - err = i8k_get_temp_type(3);
>>> + err = i8k_get_temp(3);
>>> if (err >= 0)
>>> i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4;
>>> + err = i8k_get_temp(4);
>>> + if (err >= 0)
>>> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5;
>>> + err = i8k_get_temp(5);
>>> + if (err >= 0)
>>> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6;
>>> + err = i8k_get_temp(6);
>>> + if (err >= 0)
>>> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7;
>>> + err = i8k_get_temp(7);
>>> + if (err >= 0)
>>> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8;
>>> + err = i8k_get_temp(8);
>>> + if (err >= 0)
>>> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9;
>>> + err = i8k_get_temp(9);
>>> + if (err >= 0)
>>> + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10;
>>> /* First fan attributes, if fan status or type is OK */
>>> err = i8k_get_fan_status(0);
>>>
>>
On 12/8/18 7:34 AM, Michele Sorcinelli wrote: >> Please _always_ version your patches, and add a changelog, at least if you >> would like to see it applied. I have now two patches which look almost the same, >> meaning I would have to pull both versions, compare, and hope to apply the >> correct one. > > The only change is in i8k_is_visible(): Please consider reading Documentation/process/submitting-patches.rst, chapter 2). For simplicity, I am copying the relevant paragraph here. " When you submit or resubmit a patch or patch series, include the complete patch description and justification for it. Don't just say that this is version N of the patch (series). Don't expect the subsystem maintainer to refer back to earlier patch versions or referenced URLs to find the patch description and put that into the patch. I.e., the patch (series) and its description should be self-contained. This benefits both the maintainers and reviewers. Some reviewers probably didn't even receive earlier versions of the patch. " Thanks, Guenter
On Friday 07 December 2018 20:24:49 Guenter Roeck wrote: > Anyway, I would like to get some feedback if this can cause regressions > on systems which don't support that many sensors and maybe report something > completely different if one tries to read the high-numbered sensors. > I seem to recall that there was a reason for checking the type and not just > trying to read sensor values. There can be also different problem for sensors which are turned off. E.g. on notebooks with switchable graphic cards which have included temperature sensors. When graphic card is turned off, then SMM returns error when asking for temperature value (for obvious reason). But temperature type still returns correct value "this is GPU sensors". So we cannot replace temp_type check by temp_value check. It introduce race condition between "starting GPU" and initializing dell-smm hwmon. Now linux kernel has support for dynamic turning ON/OFF switchable GPU, so we need to care about these race conditions too.
That's a valid point, I honestly didn't think about it. I opened a thread in the Dell community forum about the broken SSM procedures but I didn't get any reply. I wonder what's the best way to inform the firmware developers. https://www.dell.com/community/XPS/Broken-SMM-procedures-in-XPS-9570-firmware/m-p/6229521 On 12/10/18 10:58 AM, Pali Rohár wrote: > On Friday 07 December 2018 20:24:49 Guenter Roeck wrote: >> Anyway, I would like to get some feedback if this can cause regressions >> on systems which don't support that many sensors and maybe report something >> completely different if one tries to read the high-numbered sensors. >> I seem to recall that there was a reason for checking the type and not just >> trying to read sensor values. > > There can be also different problem for sensors which are turned off. > E.g. on notebooks with switchable graphic cards which have included > temperature sensors. When graphic card is turned off, then SMM returns > error when asking for temperature value (for obvious reason). But > temperature type still returns correct value "this is GPU sensors". > > So we cannot replace temp_type check by temp_value check. It introduce > race condition between "starting GPU" and initializing dell-smm hwmon. > Now linux kernel has support for dynamic turning ON/OFF switchable GPU, > so we need to care about these race conditions too. >
On Monday 10 December 2018 11:53:58 Michele Sorcinelli wrote:
> I wonder what's the best way to inform the firmware developers.
Mario from @Dell wrote how to report firware bugs in discussion:
https://github.com/dell/libsmbios/issues/48#issuecomment-391328501
You should have bought your machine in USA and then open ticket at:
http://www.dell.com/support/incidents-online/us/en/04
For my question how non-USA people should report bugs I have not
received answer yet. But probably similar steps, contacting Dell
support.
As far as I know Dell won't help with fixing the SMM layer, they probably changed something starting with firmware version 1.3.0 and they don't wanna release information about it. https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore I wonder if something can be done to force the discovery of the sensors in the driver, maybe adding a module option to use i8k_get_temp() as probe method as a workaround, or maybe just forcing that method for this specific model? Let me know your thoughts. Thanks, Michele. On 12/10/18 10:58 AM, Pali Rohár wrote: > On Friday 07 December 2018 20:24:49 Guenter Roeck wrote: >> Anyway, I would like to get some feedback if this can cause regressions >> on systems which don't support that many sensors and maybe report something >> completely different if one tries to read the high-numbered sensors. >> I seem to recall that there was a reason for checking the type and not just >> trying to read sensor values. > > There can be also different problem for sensors which are turned off. > E.g. on notebooks with switchable graphic cards which have included > temperature sensors. When graphic card is turned off, then SMM returns > error when asking for temperature value (for obvious reason). But > temperature type still returns correct value "this is GPU sensors". > > So we cannot replace temp_type check by temp_value check. It introduce > race condition between "starting GPU" and initializing dell-smm hwmon. > Now linux kernel has support for dynamic turning ON/OFF switchable GPU, > so we need to care about these race conditions too. >
Do you have definite response from Dell support that they are not going to fix it? Then it is pity :-( On Thursday 07 February 2019 12:16:06 Michele Sorcinelli wrote: > As far as I know Dell won't help with fixing the SMM layer, they probably > changed something starting with firmware version 1.3.0 and they don't > wanna release information about it. > > https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore > > I wonder if something can be done to force the discovery of the sensors in the driver, > maybe adding a module option to use i8k_get_temp() as probe method as a workaround, > or maybe just forcing that method for this specific model? > > Let me know your thoughts. > > Thanks, > Michele. > > On 12/10/18 10:58 AM, Pali Rohár wrote: > > On Friday 07 December 2018 20:24:49 Guenter Roeck wrote: > > > Anyway, I would like to get some feedback if this can cause regressions > > > on systems which don't support that many sensors and maybe report something > > > completely different if one tries to read the high-numbered sensors. > > > I seem to recall that there was a reason for checking the type and not just > > > trying to read sensor values. > > > > There can be also different problem for sensors which are turned off. > > E.g. on notebooks with switchable graphic cards which have included > > temperature sensors. When graphic card is turned off, then SMM returns > > error when asking for temperature value (for obvious reason). But > > temperature type still returns correct value "this is GPU sensors". > > > > So we cannot replace temp_type check by temp_value check. It introduce > > race condition between "starting GPU" and initializing dell-smm hwmon. > > Now linux kernel has support for dynamic turning ON/OFF switchable GPU, > > so we need to care about these race conditions too. > >
I think they'll never fix it or release information about the change. What if we modify the driver to fallback to i8k_get_temp() if i8k_get_temp_type() fails as a probe method? Or we could add a module option to force that behavior. Cheers, Michele. On 2/7/19 12:40 PM, Pali Rohár wrote: > Do you have definite response from Dell support that they are not going > to fix it? Then it is pity :-( > > On Thursday 07 February 2019 12:16:06 Michele Sorcinelli wrote: >> As far as I know Dell won't help with fixing the SMM layer, they probably >> changed something starting with firmware version 1.3.0 and they don't >> wanna release information about it. >> >> https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore >> >> I wonder if something can be done to force the discovery of the sensors in the driver, >> maybe adding a module option to use i8k_get_temp() as probe method as a workaround, >> or maybe just forcing that method for this specific model? >> >> Let me know your thoughts. >> >> Thanks, >> Michele. >> >> On 12/10/18 10:58 AM, Pali Rohár wrote: >>> On Friday 07 December 2018 20:24:49 Guenter Roeck wrote: >>>> Anyway, I would like to get some feedback if this can cause regressions >>>> on systems which don't support that many sensors and maybe report something >>>> completely different if one tries to read the high-numbered sensors. >>>> I seem to recall that there was a reason for checking the type and not just >>>> trying to read sensor values. >>> >>> There can be also different problem for sensors which are turned off. >>> E.g. on notebooks with switchable graphic cards which have included >>> temperature sensors. When graphic card is turned off, then SMM returns >>> error when asking for temperature value (for obvious reason). But >>> temperature type still returns correct value "this is GPU sensors". >>> >>> So we cannot replace temp_type check by temp_value check. It introduce >>> race condition between "starting GPU" and initializing dell-smm hwmon. >>> Now linux kernel has support for dynamic turning ON/OFF switchable GPU, >>> so we need to care about these race conditions too. >>> >
Looks like they've broken the SMM method for XPS 9560 too in a recent firmware upgrade. Have you thought about the possibility to fall back to i8k_get_temp() when the label method fails, or when the user decides to explicitly enable it as an alternative with a module option? Looks like Dell decided to change the API so future firmwares will stop supporting that method anyway, so I think action must be taken in the driver to address the problem. Kind regards, Michele. On 2/7/19 12:40 PM, Pali Rohár wrote: > Do you have definite response from Dell support that they are not going > to fix it? Then it is pity :-( > > On Thursday 07 February 2019 12:16:06 Michele Sorcinelli wrote: >> As far as I know Dell won't help with fixing the SMM layer, they probably >> changed something starting with firmware version 1.3.0 and they don't >> wanna release information about it. >> >> https://www.hwinfo.com/forum/Thread-Dell-XPS-15-9570-temperatures-not-named-anymore >> >> I wonder if something can be done to force the discovery of the sensors in the driver, >> maybe adding a module option to use i8k_get_temp() as probe method as a workaround, >> or maybe just forcing that method for this specific model? >> >> Let me know your thoughts. >> >> Thanks, >> Michele.
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c old mode 100644 new mode 100755 index 367a8a617..b58f756ea --- a/drivers/hwmon/dell-smm-hwmon.c +++ b/drivers/hwmon/dell-smm-hwmon.c @@ -82,9 +82,16 @@ static bool disallow_fan_support; #define I8K_HWMON_HAVE_TEMP2 (1 << 1) #define I8K_HWMON_HAVE_TEMP3 (1 << 2) #define I8K_HWMON_HAVE_TEMP4 (1 << 3) -#define I8K_HWMON_HAVE_FAN1 (1 << 4) -#define I8K_HWMON_HAVE_FAN2 (1 << 5) -#define I8K_HWMON_HAVE_FAN3 (1 << 6) +#define I8K_HWMON_HAVE_TEMP5 (1 << 4) +#define I8K_HWMON_HAVE_TEMP6 (1 << 5) +#define I8K_HWMON_HAVE_TEMP7 (1 << 6) +#define I8K_HWMON_HAVE_TEMP8 (1 << 7) +#define I8K_HWMON_HAVE_TEMP9 (1 << 8) +#define I8K_HWMON_HAVE_TEMP10 (1 << 9) + +#define I8K_HWMON_HAVE_FAN1 (1 << 10) +#define I8K_HWMON_HAVE_FAN2 (1 << 11) +#define I8K_HWMON_HAVE_FAN3 (1 << 12) MODULE_AUTHOR("Massimo Dal Zotto (dz@debian.org)"); MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>"); @@ -743,6 +750,25 @@ static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 3); static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, 3); +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 4); +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, + 4); +static SENSOR_DEVICE_ATTR(temp6_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 5); +static SENSOR_DEVICE_ATTR(temp6_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, + 5); +static SENSOR_DEVICE_ATTR(temp7_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 6); +static SENSOR_DEVICE_ATTR(temp7_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, + 6); +static SENSOR_DEVICE_ATTR(temp8_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 7); +static SENSOR_DEVICE_ATTR(temp8_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, + 7); +static SENSOR_DEVICE_ATTR(temp9_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 8); +static SENSOR_DEVICE_ATTR(temp9_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, + 8); +static SENSOR_DEVICE_ATTR(temp10_input, S_IRUGO, i8k_hwmon_show_temp, NULL, 9); +static SENSOR_DEVICE_ATTR(temp10_label, S_IRUGO, i8k_hwmon_show_temp_label, NULL, + 9); + static SENSOR_DEVICE_ATTR(fan1_input, S_IRUGO, i8k_hwmon_show_fan, NULL, 0); static SENSOR_DEVICE_ATTR(fan1_label, S_IRUGO, i8k_hwmon_show_fan_label, NULL, 0); @@ -770,15 +796,27 @@ static struct attribute *i8k_attrs[] = { &sensor_dev_attr_temp3_label.dev_attr.attr, /* 5 */ &sensor_dev_attr_temp4_input.dev_attr.attr, /* 6 */ &sensor_dev_attr_temp4_label.dev_attr.attr, /* 7 */ - &sensor_dev_attr_fan1_input.dev_attr.attr, /* 8 */ - &sensor_dev_attr_fan1_label.dev_attr.attr, /* 9 */ - &sensor_dev_attr_pwm1.dev_attr.attr, /* 10 */ - &sensor_dev_attr_fan2_input.dev_attr.attr, /* 11 */ - &sensor_dev_attr_fan2_label.dev_attr.attr, /* 12 */ - &sensor_dev_attr_pwm2.dev_attr.attr, /* 13 */ - &sensor_dev_attr_fan3_input.dev_attr.attr, /* 14 */ - &sensor_dev_attr_fan3_label.dev_attr.attr, /* 15 */ - &sensor_dev_attr_pwm3.dev_attr.attr, /* 16 */ + &sensor_dev_attr_temp5_input.dev_attr.attr, /* 8 */ + &sensor_dev_attr_temp5_label.dev_attr.attr, /* 9 */ + &sensor_dev_attr_temp6_input.dev_attr.attr, /* 10 */ + &sensor_dev_attr_temp6_label.dev_attr.attr, /* 11 */ + &sensor_dev_attr_temp7_input.dev_attr.attr, /* 12 */ + &sensor_dev_attr_temp7_label.dev_attr.attr, /* 13 */ + &sensor_dev_attr_temp8_input.dev_attr.attr, /* 14 */ + &sensor_dev_attr_temp8_label.dev_attr.attr, /* 15 */ + &sensor_dev_attr_temp9_input.dev_attr.attr, /* 16 */ + &sensor_dev_attr_temp9_label.dev_attr.attr, /* 17 */ + &sensor_dev_attr_temp10_input.dev_attr.attr, /* 18 */ + &sensor_dev_attr_temp10_label.dev_attr.attr, /* 19 */ + &sensor_dev_attr_fan1_input.dev_attr.attr, /* 20 */ + &sensor_dev_attr_fan1_label.dev_attr.attr, /* 21 */ + &sensor_dev_attr_pwm1.dev_attr.attr, /* 22 */ + &sensor_dev_attr_fan2_input.dev_attr.attr, /* 23 */ + &sensor_dev_attr_fan2_label.dev_attr.attr, /* 24 */ + &sensor_dev_attr_pwm2.dev_attr.attr, /* 25 */ + &sensor_dev_attr_fan3_input.dev_attr.attr, /* 26 */ + &sensor_dev_attr_fan3_label.dev_attr.attr, /* 27 */ + &sensor_dev_attr_pwm3.dev_attr.attr, /* 28 */ NULL }; @@ -802,13 +840,32 @@ static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr, if (index >= 6 && index <= 7 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP4)) return 0; - if (index >= 8 && index <= 10 && + if (index >= 8 && index <= 9 && + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP5)) + return 0; + if (index >= 10 && index <= 11 && + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP6)) + return 0; + if (index >= 12 && index <= 13 && + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP7)) + return 0; + if (index >= 14 && index <= 15 && + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP8)) + return 0; + if (index >= 16 && index <= 17 && + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP9)) + return 0; + if (index >= 18 && index <= 19 && + !(i8k_hwmon_flags & I8K_HWMON_HAVE_TEMP10)) + return 0; + + if (index >= 20 && index <= 22 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN1)) return 0; - if (index >= 11 && index <= 13 && + if (index >= 23 && index <= 25 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN2)) return 0; - if (index >= 14 && index <= 16 && + if (index >= 26 && index <= 28 && !(i8k_hwmon_flags & I8K_HWMON_HAVE_FAN3)) return 0; @@ -828,19 +885,37 @@ static int __init i8k_init_hwmon(void) i8k_hwmon_flags = 0; /* CPU temperature attributes, if temperature type is OK */ - err = i8k_get_temp_type(0); + err = i8k_get_temp(0); if (err >= 0) i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP1; /* check for additional temperature sensors */ - err = i8k_get_temp_type(1); + err = i8k_get_temp(1); if (err >= 0) i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP2; - err = i8k_get_temp_type(2); + err = i8k_get_temp(2); if (err >= 0) i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP3; - err = i8k_get_temp_type(3); + err = i8k_get_temp(3); if (err >= 0) i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP4; + err = i8k_get_temp(4); + if (err >= 0) + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP5; + err = i8k_get_temp(5); + if (err >= 0) + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP6; + err = i8k_get_temp(6); + if (err >= 0) + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP7; + err = i8k_get_temp(7); + if (err >= 0) + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP8; + err = i8k_get_temp(8); + if (err >= 0) + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP9; + err = i8k_get_temp(9); + if (err >= 0) + i8k_hwmon_flags |= I8K_HWMON_HAVE_TEMP10; /* First fan attributes, if fan status or type is OK */ err = i8k_get_fan_status(0);
The code has been extended to support up to 10 temperature sensors. The i8k_get_temp_type() calls inside i8k_init_hwmon() have been replaced with i8k_get_temp() as in some laptop firmwares the related SMM procedure is not implemented correctly so i8k_get_temp_type() can't be used to discover temperature sensors. Signed-off-by: Michele Sorcinelli <michelesr@autistici.org> --- drivers/hwmon/dell-smm-hwmon.c | 113 +++++++++++++++++++++++++++------ 1 file changed, 94 insertions(+), 19 deletions(-) mode change 100644 => 100755 drivers/hwmon/dell-smm-hwmon.c