Re: [4/4] hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

[Guenter Roeck]

On Sat, Jan 27, 2018 at 05:23:51PM +0100, Pali Rohár wrote:
> Measure only inlined asm code, not other functions to have as precise as
> possible measured time.
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/hwmon/dell-smm-hwmon.c | 14 ++
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index bf3bb7e1adab..e001afd53f46 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -147,14 +147,16 @@ static int i8k_smm_func(void *par)
>   int ebx = regs->ebx;
>   unsigned long duration;
>   ktime_t calltime, delta, rettime;
> -
> - calltime = ktime_get();
>  #endif
>  
>   /* SMM requires CPU 0 */
>   if (smp_processor_id() != 0)
>   return -EBUSY;
>  
> +#ifdef DEBUG
> + calltime = ktime_get();
> +#endif
> +
>  #if defined(CONFIG_X86_64)
>   asm volatile("pushq %%rax\n\t"
>   "movl 0(%%rax),%%edx\n\t"
> @@ -208,13 +210,17 @@ static int i8k_smm_func(void *par)
>   :"a"(regs)
>   :"%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
>  #endif
> - if (rc != 0 || (regs->eax & 0x) == 0x || regs->eax == eax)
> - rc = -EINVAL;
>  
>  #ifdef DEBUG
>   rettime = ktime_get();
>   delta = ktime_sub(rettime, calltime);
>   duration = ktime_to_ns(delta) >> 10;
> +#endif
> +
> + if (rc != 0 || (regs->eax & 0x) == 0x || regs->eax == eax)
> + rc = -EINVAL;
> +
> +#ifdef DEBUG

FWIW, the error introduced by dividing nS by 1,024 instead of 1,000 is
much worse than the improvements from moving the calls around. Using
specific numbers, the current code reports 500 mS as 488,281 uS.
So why bother ?

I would have suggested to use ktime_us_delta(ktime_get(), calltime)
instead to make the results more accurate. Sure, you get the offset from
the additional divide operation, but at least that would be a constant
and not a systematic error.

I'll hold this patch off for a bit. Please confirm that you want it
applied as-is. I applied the other patches from the series.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

[Oleksandr Natalenko]

Hi.

On sobota 27. ledna 2018 17:22:00 CET Pali Rohár wrote:
> This patch series disable fan support on two machines on which is BIOS
> broken. And it changes measurement of SMM calls duration.
> 
> Oleksandr Natalenko (1):
>   hwmon: (dell-smm) Disable fan support for Dell Vostro 3360
> 
> Pali Rohár (3):
>   hwmon: (dell-smm) Enable broken functionality via "force" module param
>   hwmon: (dell-smm) Disable fan support for Dell Inspiron 7720
>   hwmon: (dell-smm) Measure time duration of SMM call around inlined asm
> 
>  drivers/hwmon/dell-smm-hwmon.c | 68
> +- 1 file changed, 61
> insertions(+), 7 deletions(-)

Thanks for taking care of them (finally :)).

Regards,
  Oleksandr


--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] hwmon: (dell-smm) Disable fan support for Dell Inspiron 7720

[Pali Rohár]

Calling fan related SMM functions implemented by Dell BIOS firmware on Dell
Inspiron 7720 freeze kernel for about 500ms. Until Dell fixes it we need to
disable fan support for Dell Inspiron 7720 as it makes system unusable.

Via "force" module param fan support can be enabled.

Reported-by: vova7...@mail.ru
Signed-off-by: Pali Rohár 
Link: https://bugzilla.kernel.org/show_bug.cgi?id=195751
Cc: sta...@vger.kernel.org # v4.0+, will need backport
---
 drivers/hwmon/dell-smm-hwmon.c | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index aef4f8317ae2..3f8b4e482b64 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -76,6 +76,7 @@ static uint i8k_fan_mult = I8K_FAN_MULT;
 static uint i8k_pwm_mult;
 static uint i8k_fan_max = I8K_FAN_HIGH;
 static bool disallow_fan_type_call;
+static bool disallow_fan_support;
 
 #define I8K_HWMON_HAVE_TEMP1   (1 << 0)
 #define I8K_HWMON_HAVE_TEMP2   (1 << 1)
@@ -242,6 +243,9 @@ static int i8k_get_fan_status(int fan)
 {
struct smm_regs regs = { .eax = I8K_SMM_GET_FAN, };
 
+   if (disallow_fan_support)
+   return -EINVAL;
+
regs.ebx = fan & 0xff;
return i8k_smm() ? : regs.eax & 0xff;
 }
@@ -253,6 +257,9 @@ static int i8k_get_fan_speed(int fan)
 {
struct smm_regs regs = { .eax = I8K_SMM_GET_SPEED, };
 
+   if (disallow_fan_support)
+   return -EINVAL;
+
regs.ebx = fan & 0xff;
return i8k_smm() ? : (regs.eax & 0x) * i8k_fan_mult;
 }
@@ -264,7 +271,7 @@ static int _i8k_get_fan_type(int fan)
 {
struct smm_regs regs = { .eax = I8K_SMM_GET_FAN_TYPE, };
 
-   if (disallow_fan_type_call)
+   if (disallow_fan_support || disallow_fan_type_call)
return -EINVAL;
 
regs.ebx = fan & 0xff;
@@ -289,6 +296,9 @@ static int i8k_get_fan_nominal_speed(int fan, int speed)
 {
struct smm_regs regs = { .eax = I8K_SMM_GET_NOM_SPEED, };
 
+   if (disallow_fan_support)
+   return -EINVAL;
+
regs.ebx = (fan & 0xff) | (speed << 8);
return i8k_smm() ? : (regs.eax & 0x) * i8k_fan_mult;
 }
@@ -300,6 +310,9 @@ static int i8k_set_fan(int fan, int speed)
 {
struct smm_regs regs = { .eax = I8K_SMM_SET_FAN, };
 
+   if (disallow_fan_support)
+   return -EINVAL;
+
speed = (speed < 0) ? 0 : ((speed > i8k_fan_max) ? i8k_fan_max : speed);
regs.ebx = (fan & 0xff) | (speed << 8);
 
@@ -772,6 +785,8 @@ static struct attribute *i8k_attrs[] = {
 static umode_t i8k_is_visible(struct kobject *kobj, struct attribute *attr,
  int index)
 {
+   if (disallow_fan_support && index >= 8)
+   return 0;
if (disallow_fan_type_call &&
(index == 9 || index == 12 || index == 15))
return 0;
@@ -1039,6 +1054,23 @@ static const struct dmi_system_id 
i8k_blacklist_fan_type_dmi_table[] __initconst
 };
 
 /*
+ * On some machines all fan related SMM functions implemented by Dell BIOS
+ * firmware freeze kernel for about 500ms. Until Dell fixes these problems fan
+ * support for affected blacklisted Dell machines stay disabled.
+ * See bug: https://bugzilla.kernel.org/show_bug.cgi?id=195751
+ */
+static struct dmi_system_id i8k_blacklist_fan_support_dmi_table[] __initdata = 
{
+   {
+   .ident = "Dell Inspiron 7720",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Inspiron 7720"),
+   },
+   },
+   { }
+};
+
+/*
  * Probe for the presence of a supported laptop.
  */
 static int __init i8k_probe(void)
@@ -1060,6 +1092,12 @@ static int __init i8k_probe(void)
i8k_get_dmi_data(DMI_BIOS_VERSION));
}
 
+   if (dmi_check_system(i8k_blacklist_fan_support_dmi_table)) {
+   pr_warn("broken Dell BIOS detected, disallow fan support\n");
+   if (!force)
+   disallow_fan_support = true;
+   }
+
if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
pr_warn("broken Dell BIOS detected, disallow fan type call\n");
if (!force)
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hwmon: (sht21) Don't use clock stretching

[Guenter Roeck]

On 01/26/2018 03:37 PM, Danilo Bargen wrote:

The current driver uses clock stretching (hold master) mode to retrieve
the measurements. This blocks the bus and may cause problems on some
systems like the BCM2835 (Raspberry Pi) that have buggy I2C hardware¹.

¹ http://www.advamation.com/knowhow/raspberrypi/rpi-i2c-bug.html

Signed-off-by: Danilo Bargen 
Reviewed-by: Andreas Brauchli 
---
  drivers/hwmon/sht21.c | 79 ++-
  1 file changed, 60 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/sht21.c b/drivers/hwmon/sht21.c
index 984f14b3e89a..56aa13b15682 100644
--- a/drivers/hwmon/sht21.c
+++ b/drivers/hwmon/sht21.c
@@ -25,15 +25,24 @@
  #include 
  #include 
  #include 
+#include 
  
-/* I2C command bytes */

-#define SHT21_TRIG_T_MEASUREMENT_HM  0xe3
-#define SHT21_TRIG_RH_MEASUREMENT_HM 0xe5
+/* I2C commands */
+static const char sht21_trig_t_measurement_nh[] = { 0xf3 };
+static const char sht21_trig_rh_measurement_nh[] = { 0xf5 };
  #define SHT21_READ_SNB_CMD1 0xFA
  #define SHT21_READ_SNB_CMD2 0x0F
  #define SHT21_READ_SNAC_CMD1 0xFC
  #define SHT21_READ_SNAC_CMD2 0xC9
  
+/* I2C lengths */

+#define SHT21_CMD_LENGTH 1
+#define SHT21_RESPONSE_LENGTH 3
+
+/* Max waiting times in ms at max resolution, datasheet table 7 */
+#define SHT21_WAIT_T 85
+#define SHT21_WAIT_RH 29
+
  /**
   * struct sht21 - SHT21 device specific data
   * @hwmon_dev: device registered with hwmon
@@ -93,6 +102,7 @@ static inline int sht21_rh_ticks_to_per_cent_mille(int ticks)
  static int sht21_update_measurements(struct device *dev)
  {
int ret = 0;
+   unsigned char buf[SHT21_RESPONSE_LENGTH];
struct sht21 *sht21 = dev_get_drvdata(dev);
struct i2c_client *client = sht21->client;
  
@@ -103,22 +113,60 @@ static int sht21_update_measurements(struct device *dev)

 * maximum two measurements per second at 12bit accuracy shall be made.
 */
if (time_after(jiffies, sht21->last_update + HZ / 2) || !sht21->valid) {
-   ret = i2c_smbus_read_word_swapped(client,
- SHT21_TRIG_T_MEASUREMENT_HM);
-   if (ret < 0)
+   /*
+* Read from the SHT21 in non blocking mode
+* (no clock stretching).
+*/
+
+   /* Trigger temperature measurement */
+   ret = i2c_master_send(client,
+ sht21_trig_t_measurement_nh,
+ SHT21_CMD_LENGTH);
+   if (ret != SHT21_CMD_LENGTH) {
+   dev_err(dev, "failed to send command: %d\n", ret);
+   ret = -EIO;


Both i2c_master_send() and i2c_master_recv() return either the expected length
or an error. Overriding the error is neither necessary nor desirable.


+   goto out;
+   }
+   msleep(SHT21_WAIT_T);
+
+   /* Read temperature data */
+   ret = i2c_master_recv(client, buf, sizeof(buf));
+   if (ret != SHT21_RESPONSE_LENGTH) {
+   dev_err(dev, "failed to read values: %d\n", ret);
+   ret = -EIO;
+   goto out;
+   }
+   sht21->temperature = sht21_temp_ticks_to_millicelsius(
+   (buf[0] << 8) | buf[1]
+   );


Please find a better alignment than that.


+
+   /* Trigger humidity measurement */
+   ret = i2c_master_send(client,
+ sht21_trig_rh_measurement_nh,
+ SHT21_CMD_LENGTH);
+   if (ret != SHT21_CMD_LENGTH) {
+   dev_err(dev, "failed to send command: %d\n", ret);
+   ret = -EIO;
goto out;
-   sht21->temperature = sht21_temp_ticks_to_millicelsius(ret);
-   ret = i2c_smbus_read_word_swapped(client,
- SHT21_TRIG_RH_MEASUREMENT_HM);
-   if (ret < 0)
+   }
+   msleep(SHT21_WAIT_RH);
+
+   /* Read humidity data */
+   ret = i2c_master_recv(client, buf, sizeof(buf));
+   if (ret != SHT21_RESPONSE_LENGTH) {
+   dev_err(dev, "failed to read values: %d\n", ret);
+   ret = -EIO;
goto out;
-   sht21->humidity = sht21_rh_ticks_to_per_cent_mille(ret);
+   }
+   sht21->humidity = sht21_rh_ticks_to_per_cent_mille(
+   (buf[0] << 8) | buf[1]
+   );
+
sht21->last_update = jiffies;
sht21->valid = 1;
}
  out:
mutex_unlock(>lock);
-
return ret >= 0 ? 0 : ret;
  }
  
@@ -269,13 +317,6 @@ static int sht21_probe(struct i2c_client 

[PATCH 4/4] hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

[Pali Rohár]

Measure only inlined asm code, not other functions to have as precise as
possible measured time.

Signed-off-by: Pali Rohár 
---
 drivers/hwmon/dell-smm-hwmon.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index bf3bb7e1adab..e001afd53f46 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -147,14 +147,16 @@ static int i8k_smm_func(void *par)
int ebx = regs->ebx;
unsigned long duration;
ktime_t calltime, delta, rettime;
-
-   calltime = ktime_get();
 #endif
 
/* SMM requires CPU 0 */
if (smp_processor_id() != 0)
return -EBUSY;
 
+#ifdef DEBUG
+   calltime = ktime_get();
+#endif
+
 #if defined(CONFIG_X86_64)
asm volatile("pushq %%rax\n\t"
"movl 0(%%rax),%%edx\n\t"
@@ -208,13 +210,17 @@ static int i8k_smm_func(void *par)
:"a"(regs)
:"%ebx", "%ecx", "%edx", "%esi", "%edi", "memory");
 #endif
-   if (rc != 0 || (regs->eax & 0x) == 0x || regs->eax == eax)
-   rc = -EINVAL;
 
 #ifdef DEBUG
rettime = ktime_get();
delta = ktime_sub(rettime, calltime);
duration = ktime_to_ns(delta) >> 10;
+#endif
+
+   if (rc != 0 || (regs->eax & 0x) == 0x || regs->eax == eax)
+   rc = -EINVAL;
+
+#ifdef DEBUG
pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x  (took %7lu usecs)\n", eax, ebx,
(rc ? 0x : regs->eax & 0x), duration);
 #endif
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] hwmon: (dell-smm) Disable fan support on Inspiron 7720 and Vostro 3360

[Pali Rohár]

This patch series disable fan support on two machines on which is BIOS
broken. And it changes measurement of SMM calls duration.

Oleksandr Natalenko (1):
  hwmon: (dell-smm) Disable fan support for Dell Vostro 3360

Pali Rohár (3):
  hwmon: (dell-smm) Enable broken functionality via "force" module param
  hwmon: (dell-smm) Disable fan support for Dell Inspiron 7720
  hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

 drivers/hwmon/dell-smm-hwmon.c | 68 +-
 1 file changed, 61 insertions(+), 7 deletions(-)

-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] hwmon: (dell-smm) Enable broken functionality via "force" module param

[Pali Rohár]

Some Dell machines are broken and some functionality is disabled. Show
warning into dmesg about this fact and allow user via "force" module param
to override brokenness and enable broken functionality.

Signed-off-by: Pali Rohár 
---
 drivers/hwmon/dell-smm-hwmon.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index c7c9e95e58a8..aef4f8317ae2 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -1060,8 +1060,11 @@ static int __init i8k_probe(void)
i8k_get_dmi_data(DMI_BIOS_VERSION));
}
 
-   if (dmi_check_system(i8k_blacklist_fan_type_dmi_table))
-   disallow_fan_type_call = true;
+   if (dmi_check_system(i8k_blacklist_fan_type_dmi_table)) {
+   pr_warn("broken Dell BIOS detected, disallow fan type call\n");
+   if (!force)
+   disallow_fan_type_call = true;
+   }
 
strlcpy(bios_version, i8k_get_dmi_data(DMI_BIOS_VERSION),
sizeof(bios_version));
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: i8k_smm_func() takes enormous of time to execute

[Guenter Roeck]

On 01/27/2018 06:09 AM, Pali Rohár wrote:

On Monday 04 December 2017 13:18:29 Pali Rohár wrote:

Your last patch in bugzilla looks ok, you add my Reviewed-by: Pali
Rohár 


Could you please advice on how to proceed further? I can submit all 3
patches (incl. yours two), to a ML.


Now it is up to hwmon maintainers (Jean Delvare & Guenter Roeck) to pick
up these patches. I think nothing more is needed from your side. (And if
yes, Jean would write.)



Jean Delvare & Guenter Roeck: PING

Can you process patches from that bugzilla ticket?
https://bugzilla.kernel.org/show_bug.cgi?id=195751


No. You should know better. Please follow the established process for
submitting patches into the upstream kernel.

Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: i8k_smm_func() takes enormous of time to execute

[Pali Rohár]

On Monday 04 December 2017 13:18:29 Pali Rohár wrote:
> > > Your last patch in bugzilla looks ok, you add my Reviewed-by: Pali
> > > Rohár 
> > 
> > Could you please advice on how to proceed further? I can submit all 3
> > patches (incl. yours two), to a ML.
> 
> Now it is up to hwmon maintainers (Jean Delvare & Guenter Roeck) to pick
> up these patches. I think nothing more is needed from your side. (And if
> yes, Jean would write.)
> 

Jean Delvare & Guenter Roeck: PING

Can you process patches from that bugzilla ticket?
https://bugzilla.kernel.org/show_bug.cgi?id=195751

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: PGP signature