synaptics: match PNP-Id is not sufficient for min/max quirks

synaptics: match PNP-Id is not sufficient for min/max quirks

[Daniel Martin]

Hi folks,

these patches fix the problem described at:
    https://bugzilla.kernel.org/show_bug.cgi?id=91541#c0

I verified that the following models have the min/max dimensions we want
them to have afterwards:
    T440s (2013 and 2014),
    T440p (2014),
    T540p (2013 and 2014),
    X240 (2013),
    Helix (2013?) and
    X1 Carbon (2013, LEN0030).


Any comments are welcome,
    Daniel Martin

[PATCH 1/6] Input: synaptics - Split synaptics_resolution(), query first

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Split the function synaptics_resolution() into
    synaptics_resolution() and synaptics_quirks().

synaptics_resolution() will be called before synaptics_quirks() to query
dimensions and resolutions before overwriting them with quirks.

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f947292..88f02aa 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -342,7 +342,6 @@ static int synaptics_resolution(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
 	unsigned char resp[3];
-	int i;
 
 	if (SYN_ID_MAJOR(priv->identity) < 4)
 		return 0;
@@ -354,17 +353,6 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		}
 	}
 
-	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
-		if (psmouse_matches_pnp_id(psmouse,
-					   min_max_pnpid_table[i].pnp_ids)) {
-			priv->x_min = min_max_pnpid_table[i].x_min;
-			priv->x_max = min_max_pnpid_table[i].x_max;
-			priv->y_min = min_max_pnpid_table[i].y_min;
-			priv->y_max = min_max_pnpid_table[i].y_max;
-			return 0;
-		}
-	}
-
 	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 5 &&
 	    SYN_CAP_MAX_DIMENSIONS(priv->ext_cap_0c)) {
 		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MAX_COORDS, resp)) {
@@ -390,6 +378,32 @@ static int synaptics_resolution(struct psmouse *psmouse)
 	return 0;
 }
 
+/*
+ * Apply quirk(s) if the hardware matches
+ */
+
+static int synaptics_quirks(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int i;
+
+	if (SYN_ID_MAJOR(priv->identity) < 4)
+		return 0;
+
+	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
+		if (psmouse_matches_pnp_id(psmouse,
+					   min_max_pnpid_table[i].pnp_ids)) {
+			priv->x_min = min_max_pnpid_table[i].x_min;
+			priv->x_max = min_max_pnpid_table[i].x_max;
+			priv->y_min = min_max_pnpid_table[i].y_min;
+			priv->y_max = min_max_pnpid_table[i].y_max;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static int synaptics_query_hardware(struct psmouse *psmouse)
 {
 	if (synaptics_identify(psmouse))
@@ -404,6 +418,8 @@ static int synaptics_query_hardware(struct psmouse *psmouse)
 		return -1;
 	if (synaptics_resolution(psmouse))
 		return -1;
+	if (synaptics_quirks(psmouse))
+		return -1;
 
 	return 0;
 }
-- 
2.2.2

[PATCH 2/6] Input: synaptics - Log queried and quirked dimension values

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Logging the dimension values we queried (info) and the values we use
from a quirk to overwrite (warn) can be helpful for debugging.

This partly relates to bug:
    https://bugzilla.kernel.org/show_bug.cgi?id=91541

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 88f02aa..e2b4f5a 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -361,6 +361,10 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		} else {
 			priv->x_max = (resp[0] << 5) | ((resp[1] & 0x0f) << 1);
 			priv->y_max = (resp[2] << 5) | ((resp[1] & 0xf0) >> 3);
+			psmouse_info(psmouse,
+				     "queried max coordinates: "
+				     "x [..%d], y [..%d]\n",
+				     priv->x_max, priv->y_max);
 		}
 	}
 
@@ -372,6 +376,10 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		} else {
 			priv->x_min = (resp[0] << 5) | ((resp[1] & 0x0f) << 1);
 			priv->y_min = (resp[2] << 5) | ((resp[1] & 0xf0) >> 3);
+			psmouse_info(psmouse,
+				     "queried min coordinates: "
+				     "x [%d..], y [%d..]\n",
+				     priv->x_min, priv->y_min);
 		}
 	}
 
@@ -397,6 +405,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
 			priv->x_max = min_max_pnpid_table[i].x_max;
 			priv->y_min = min_max_pnpid_table[i].y_min;
 			priv->y_max = min_max_pnpid_table[i].y_max;
+			psmouse_warn(psmouse,
+				     "quirked min/max coordinates: "
+				     "x [%d..%d], y [%d..%d]\n",
+				     priv->x_min, priv->x_max,
+				     priv->y_min, priv->y_max);
 			break;
 		}
 	}
-- 
2.2.2

[PATCH 3/6] Input: synaptics - Query min dimensions when safe firmware

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Query the min dimensions even if the check
    SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
fails, but we know that the firmware version is safe. Atm. this is
version 8.1 only.

With that we don't need quirks for post-2013 models anymore as they
expose correct min and max dimensions.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index e2b4f5a..8ed4760 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
 	return -1;
 }
 
+struct fw_version {
+	unsigned char major;
+	unsigned char minor;
+};
+
+static const struct fw_version safe_fw_list[] = {
+	{8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
+	{ }
+};
+
+static bool synaptics_is_safe_fw(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int i;
+
+	for (i = 0; safe_fw_list[i].major; i++) {
+		if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
+		    safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * Read touchpad resolution and maximum reported coordinates
  * Resolution is left zero if touchpad does not support the query
@@ -368,7 +392,8 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		}
 	}
 
-	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
+	if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 ||
+	     synaptics_is_safe_fw(psmouse)) &&
 	    SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
 		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
 			psmouse_warn(psmouse,
-- 
2.2.2

[PATCH 4/6] Input: synaptics - Skip quirks when post-2013 dimensions

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
have post-2013 model and don't need to apply any quirk.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 8ed4760..7f6e004 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -423,6 +423,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
 	if (SYN_ID_MAJOR(priv->identity) < 4)
 		return 0;
 
+	/* Post-2013 models expose correct dimensions. */
+	if (priv->x_min == 1266 && priv->x_max == 5674 &&
+	    priv->y_min == 1170 && priv->y_max == 4684)
+		return 0;
+
 	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
 		if (psmouse_matches_pnp_id(psmouse,
 					   min_max_pnpid_table[i].pnp_ids)) {
-- 
2.2.2

[PATCH 5/6] Input: synaptics - Add PnP id for X1 Carbon 1nd

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

This adds the PnP id (LEN0030) for the X1 Carbon 1nd (2013/07) to the
list of known PnP ids.

The firmware exposes the correct dimensions:
    x [1264..5678], y [1124..4732].

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 7f6e004..70480c0 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -161,6 +161,7 @@ static const char * const topbuttonpad_pnp_ids[] = {
 	"LEN002C",
 	"LEN002D",
 	"LEN002E",
+	"LEN0030", /* X1 Carbon 1nd */
 	"LEN0033", /* Helix */
 	"LEN0034", /* T431s, L440, L540, T540, W540, X1 Carbon 2nd */
 	"LEN0035", /* X240 */
-- 
2.2.2

[PATCH 6/6] Input: synaptics - Remove obsolete min/max quirk for X240

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

The firmware of the X240 (LEN0035, 2013/12) exposes the same values
    x [1232..5710], y [1156..4696]
as the quirk applies.

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 70480c0..4754fa6 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -131,7 +131,7 @@ static const struct min_max_quirk min_max_pnpid_table[] = {
 		1024, 5052, 2258, 4832
 	},
 	{
-		(const char * const []){"LEN0035", "LEN0042", NULL},
+		(const char * const []){"LEN0042", NULL},
 		1232, 5710, 1156, 4696
 	},
 	{
-- 
2.2.2

Re: [PATCH 5/6] Input: synaptics - Add PnP id for X1 Carbon 1nd

[Daniel Martin]

On 22 January 2015 at 17:46, Daniel Martin <daniel.martin@secunet.com> wrote:
> From: Daniel Martin <consume.noise@gmail.com>
>
> This adds the PnP id (LEN0030) for the X1 Carbon 1nd (2013/07) to the
> list of known PnP ids.
>
> The firmware exposes the correct dimensions:
>     x [1264..5678], y [1124..4732].
>
> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> ---

Ups, forgotten to mention that I _think_ that this is the 1nd generation.

>  drivers/input/mouse/synaptics.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 7f6e004..70480c0 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -161,6 +161,7 @@ static const char * const topbuttonpad_pnp_ids[] = {
>         "LEN002C",
>         "LEN002D",
>         "LEN002E",
> +       "LEN0030", /* X1 Carbon 1nd */
>         "LEN0033", /* Helix */
>         "LEN0034", /* T431s, L440, L540, T540, W540, X1 Carbon 2nd */
>         "LEN0035", /* X240 */
> --
> 2.2.2
>

Re: [PATCH 5/6] Input: synaptics - Add PnP id for X1 Carbon 1nd

[Daniel Martin]

Withdrawn!

On 22 January 2015 at 17:46, Daniel Martin <daniel.martin@secunet.com> wrote:
> From: Daniel Martin <consume.noise@gmail.com>
>
> This adds the PnP id (LEN0030) for the X1 Carbon 1nd (2013/07) to the
> list of known PnP ids.
>
> The firmware exposes the correct dimensions:
>     x [1264..5678], y [1124..4732].
>
> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 7f6e004..70480c0 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -161,6 +161,7 @@ static const char * const topbuttonpad_pnp_ids[] = {
>         "LEN002C",
>         "LEN002D",
>         "LEN002E",
> +       "LEN0030", /* X1 Carbon 1nd */
>         "LEN0033", /* Helix */
>         "LEN0034", /* T431s, L440, L540, T540, W540, X1 Carbon 2nd */
>         "LEN0035", /* X240 */

The list is clearly named topbuttonpad_pnp_ids and therefore this
model shouldn't be listed there. But, I did it. *facepalm*

[v2] synaptics: match PNP-Id is not sufficient for min/max quirks

[Daniel Martin]

Hi,

this is the second version of my patches to address the problems
described at:
    https://bugzilla.kernel.org/show_bug.cgi?id=91541

I have incorporated changes according to Hans comments. That means I
removed the patch
    Input: synaptics - Add PnP id for X1 Carbon 1nd
as the PnP id doesn't belong into the list topbuttonpad_pnp_ids and in
the first patch
    Input: synaptics - Split synaptics_resolution(), query ...
I removed the SYN_ID_MAJOR() major check.

Is there anything else I should change?


Cheers,
    Daniel

[PATCH v2 1/5] Input: synaptics - Split synaptics_resolution(), query first

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Split the function synaptics_resolution() into
    synaptics_resolution() and synaptics_quirks().

synaptics_resolution() will be called before synaptics_quirks() to query
dimensions and resolutions before overwriting them with quirks.

v2: Removed SYN_ID_MAJOR() check from synaptics_quirks().

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f947292..89b408b 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -342,7 +342,6 @@ static int synaptics_resolution(struct psmouse *psmouse)
 {
 	struct synaptics_data *priv = psmouse->private;
 	unsigned char resp[3];
-	int i;
 
 	if (SYN_ID_MAJOR(priv->identity) < 4)
 		return 0;
@@ -354,17 +353,6 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		}
 	}
 
-	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
-		if (psmouse_matches_pnp_id(psmouse,
-					   min_max_pnpid_table[i].pnp_ids)) {
-			priv->x_min = min_max_pnpid_table[i].x_min;
-			priv->x_max = min_max_pnpid_table[i].x_max;
-			priv->y_min = min_max_pnpid_table[i].y_min;
-			priv->y_max = min_max_pnpid_table[i].y_max;
-			return 0;
-		}
-	}
-
 	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 5 &&
 	    SYN_CAP_MAX_DIMENSIONS(priv->ext_cap_0c)) {
 		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MAX_COORDS, resp)) {
@@ -390,6 +378,29 @@ static int synaptics_resolution(struct psmouse *psmouse)
 	return 0;
 }
 
+/*
+ * Apply quirk(s) if the hardware matches
+ */
+
+static int synaptics_quirks(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int i;
+
+	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
+		if (psmouse_matches_pnp_id(psmouse,
+					   min_max_pnpid_table[i].pnp_ids)) {
+			priv->x_min = min_max_pnpid_table[i].x_min;
+			priv->x_max = min_max_pnpid_table[i].x_max;
+			priv->y_min = min_max_pnpid_table[i].y_min;
+			priv->y_max = min_max_pnpid_table[i].y_max;
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static int synaptics_query_hardware(struct psmouse *psmouse)
 {
 	if (synaptics_identify(psmouse))
@@ -404,6 +415,8 @@ static int synaptics_query_hardware(struct psmouse *psmouse)
 		return -1;
 	if (synaptics_resolution(psmouse))
 		return -1;
+	if (synaptics_quirks(psmouse))
+		return -1;
 
 	return 0;
 }
-- 
2.2.2

[PATCH v2 2/5] Input: synaptics - Log queried and quirked dimension values

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Logging the dimension values we queried (info) and the values we use
from a quirk to overwrite (warn) can be helpful for debugging.

This partly relates to bug:
    https://bugzilla.kernel.org/show_bug.cgi?id=91541

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 89b408b..7a78d75 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -361,6 +361,10 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		} else {
 			priv->x_max = (resp[0] << 5) | ((resp[1] & 0x0f) << 1);
 			priv->y_max = (resp[2] << 5) | ((resp[1] & 0xf0) >> 3);
+			psmouse_info(psmouse,
+				     "queried max coordinates: "
+				     "x [..%d], y [..%d]\n",
+				     priv->x_max, priv->y_max);
 		}
 	}
 
@@ -372,6 +376,10 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		} else {
 			priv->x_min = (resp[0] << 5) | ((resp[1] & 0x0f) << 1);
 			priv->y_min = (resp[2] << 5) | ((resp[1] & 0xf0) >> 3);
+			psmouse_info(psmouse,
+				     "queried min coordinates: "
+				     "x [%d..], y [%d..]\n",
+				     priv->x_min, priv->y_min);
 		}
 	}
 
@@ -394,6 +402,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
 			priv->x_max = min_max_pnpid_table[i].x_max;
 			priv->y_min = min_max_pnpid_table[i].y_min;
 			priv->y_max = min_max_pnpid_table[i].y_max;
+			psmouse_warn(psmouse,
+				     "quirked min/max coordinates: "
+				     "x [%d..%d], y [%d..%d]\n",
+				     priv->x_min, priv->x_max,
+				     priv->y_min, priv->y_max);
 			break;
 		}
 	}
-- 
2.2.2

[PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

Query the min dimensions even if the check
    SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
fails, but we know that the firmware version is safe. Atm. this is
version 8.1 only.

With that we don't need quirks for post-2013 models anymore as they
expose correct min and max dimensions.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 7a78d75..37d4dff 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
 	return -1;
 }
 
+struct fw_version {
+	unsigned char major;
+	unsigned char minor;
+};
+
+static const struct fw_version safe_fw_list[] = {
+	{8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
+	{ }
+};
+
+static bool synaptics_is_safe_fw(struct psmouse *psmouse)
+{
+	struct synaptics_data *priv = psmouse->private;
+	int i;
+
+	for (i = 0; safe_fw_list[i].major; i++) {
+		if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
+		    safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
+			return true;
+	}
+
+	return false;
+}
+
 /*
  * Read touchpad resolution and maximum reported coordinates
  * Resolution is left zero if touchpad does not support the query
@@ -368,7 +392,8 @@ static int synaptics_resolution(struct psmouse *psmouse)
 		}
 	}
 
-	if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
+	if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 ||
+	     synaptics_is_safe_fw(psmouse)) &&
 	    SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
 		if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
 			psmouse_warn(psmouse,
-- 
2.2.2

[PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
have post-2013 model and don't need to apply any quirk.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 37d4dff..f6c43ff 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
 	struct synaptics_data *priv = psmouse->private;
 	int i;
 
+	/* Post-2013 models expose correct dimensions. */
+	if (priv->x_min == 1266 && priv->x_max == 5674 &&
+	    priv->y_min == 1170 && priv->y_max == 4684)
+		return 0;
+
 	for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
 		if (psmouse_matches_pnp_id(psmouse,
 					   min_max_pnpid_table[i].pnp_ids)) {
-- 
2.2.2

[PATCH v2 5/5] Input: synaptics - Remove obsolete min/max quirk for X240

[Daniel Martin]

From: Daniel Martin <consume.noise@gmail.com>

The firmware of the X240 (LEN0035, 2013/12) exposes the same values
    x [1232..5710], y [1156..4696]
as the quirk applies.

Signed-off-by: Daniel Martin <consume.noise@gmail.com>
---
 drivers/input/mouse/synaptics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index f6c43ff..97b0e77 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -131,7 +131,7 @@ static const struct min_max_quirk min_max_pnpid_table[] = {
 		1024, 5052, 2258, 4832
 	},
 	{
-		(const char * const []){"LEN0035", "LEN0042", NULL},
+		(const char * const []){"LEN0042", NULL},
 		1232, 5710, 1156, 4696
 	},
 	{
-- 
2.2.2

Re: [v2] synaptics: match PNP-Id is not sufficient for min/max quirks

[Hans de Goede]

Hi,

On 01/27/2015 09:34 AM, Daniel Martin wrote:
> Hi,
>
> this is the second version of my patches to address the problems
> described at:
>      https://bugzilla.kernel.org/show_bug.cgi?id=91541
>
> I have incorporated changes according to Hans comments. That means I
> removed the patch
>      Input: synaptics - Add PnP id for X1 Carbon 1nd
> as the PnP id doesn't belong into the list topbuttonpad_pnp_ids and in
> the first patch
>      Input: synaptics - Split synaptics_resolution(), query ...
> I removed the SYN_ID_MAJOR() major check.

Patch set looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

> Is there anything else I should change?

That is up to Dmitry, btw when sending input patches you should send
them directly to Dmitry with the list in the Cc, I've added Dmitry to
the Cc now.

Regards,

Hans

Re: [PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Benjamin Tissoires]

Hi Daniel,

in one hand I would like to see that upstream ASAP. But on the other
hand, I have some concerns here and there regarding this series.

On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
<daniel.martin@secunet.com> wrote:
> From: Daniel Martin <consume.noise@gmail.com>
>
> Query the min dimensions even if the check
>     SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
> fails, but we know that the firmware version is safe. Atm. this is
> version 8.1 only.
>
> With that we don't need quirks for post-2013 models anymore as they
> expose correct min and max dimensions.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 7a78d75..37d4dff 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
>         return -1;
>  }
>
> +struct fw_version {
> +       unsigned char major;
> +       unsigned char minor;
> +};
> +
> +static const struct fw_version safe_fw_list[] = {
> +       {8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
> +       { }
> +};
> +
> +static bool synaptics_is_safe_fw(struct psmouse *psmouse)
> +{
> +       struct synaptics_data *priv = psmouse->private;
> +       int i;
> +
> +       for (i = 0; safe_fw_list[i].major; i++) {
> +               if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
> +                   safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
> +                       return true;
> +       }
> +
> +       return false;
> +}

This seems a little bit too much for just one firmware ID.
Plus the name safe_fw_list gives hope that this list can be used for
something else later, but then it may conflict with it current
purpose.

How about we just rely on the current list of PNPIDs we already have
for this generation?
This might not be a good idea either, so an other solution would be to
directly check for firmware 8.1 in the test below.

I'd like to hear other opinions on this however before we commit to a decision.


Cheers,
Benjamin

PS: I think Hans already mentioned, but if you want your patches to
land in Dmitry's tree, it's better to CC him directly when submitting
a patch.

> +
>  /*
>   * Read touchpad resolution and maximum reported coordinates
>   * Resolution is left zero if touchpad does not support the query
> @@ -368,7 +392,8 @@ static int synaptics_resolution(struct psmouse *psmouse)
>                 }
>         }
>
> -       if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
> +       if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 ||
> +            synaptics_is_safe_fw(psmouse)) &&
>             SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
>                 if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
>                         psmouse_warn(psmouse,
> --
> 2.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Benjamin Tissoires]

Hi Daniel,

On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
<daniel.martin@secunet.com> wrote:
> From: Daniel Martin <consume.noise@gmail.com>
>
> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
> have post-2013 model and don't need to apply any quirk.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> ---
>  drivers/input/mouse/synaptics.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 37d4dff..f6c43ff 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>         struct synaptics_data *priv = psmouse->private;
>         int i;
>
> +       /* Post-2013 models expose correct dimensions. */
> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
> +           priv->y_min == 1170 && priv->y_max == 4684)
> +               return 0;
> +

Well, this one, I don't like it either :(

At least, the test should be within the psmouse_matches_pnp_id() below
to ensure we are deciding with Lenovo devices only.

The other concern is hardcoding these values in the code directly.
What if Synaptics/Lenovo decides to ship a new released model with
proper min_max ranges but with a different offset?

Andrew told us that the board ID should be enough to discriminate old
and faulty touchpads from the new and valid touchpads.

My concern here is that we will have to backport these changes in the
various stable kernel and the various distributions. And if we do not
end up with the right solution right now, that means that we will have
to do the job over and over.

I am quite tempted to find a solution in the userspace for that fix.
Not sure I'll be able to find the right one right now, but it may
worth trying.

Cheers,
Benjamin

>         for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
>                 if (psmouse_matches_pnp_id(psmouse,
>                                            min_max_pnpid_table[i].pnp_ids)) {
> --
> 2.2.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Benjamin Tissoires]

On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi Daniel,
>
> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
> <daniel.martin@secunet.com> wrote:
>> From: Daniel Martin <consume.noise@gmail.com>
>>
>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>> have post-2013 model and don't need to apply any quirk.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>> ---
>>  drivers/input/mouse/synaptics.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 37d4dff..f6c43ff 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>         struct synaptics_data *priv = psmouse->private;
>>         int i;
>>
>> +       /* Post-2013 models expose correct dimensions. */
>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>> +           priv->y_min == 1170 && priv->y_max == 4684)
>> +               return 0;
>> +
>
> Well, this one, I don't like it either :(
>
> At least, the test should be within the psmouse_matches_pnp_id() below
> to ensure we are deciding with Lenovo devices only.
>
> The other concern is hardcoding these values in the code directly.
> What if Synaptics/Lenovo decides to ship a new released model with
> proper min_max ranges but with a different offset?
>
> Andrew told us that the board ID should be enough to discriminate old
> and faulty touchpads from the new and valid touchpads.
>
> My concern here is that we will have to backport these changes in the
> various stable kernel and the various distributions. And if we do not
> end up with the right solution right now, that means that we will have
> to do the job over and over.
>
> I am quite tempted to find a solution in the userspace for that fix.
> Not sure I'll be able to find the right one right now, but it may
> worth trying.
>

So, the user space solution seems difficult because we do not export
either the board_id or the firmware_id. So that would required to
update the kernel anyway, a bunch of user space tools and a hwdb... :(

How about we just add an extra min/max in struct min_max_quirk,
compare the current min/max with the 2 possible values and if there is
a match, we do not override the values.
This way, we keep the crap of wrong/correct min max in the small list
of device we know are problematic, and if the new batch of E540 has a
different correct min/max range, then we will be able to adjust it
without breaking the other we fixed.

Dmitry, Hans, any comments on this?

Cheers,
Benjamin

>>         for (i = 0; min_max_pnpid_table[i].pnp_ids; i++) {
>>                 if (psmouse_matches_pnp_id(psmouse,
>>                                            min_max_pnpid_table[i].pnp_ids)) {
>> --
>> 2.2.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Hans de Goede]

Hi,

On 01/29/2015 07:48 PM, Benjamin Tissoires wrote:
> Hi Daniel,
> 
> in one hand I would like to see that upstream ASAP. But on the other
> hand, I have some concerns here and there regarding this series.
> 
> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
> <daniel.martin@secunet.com> wrote:
>> From: Daniel Martin <consume.noise@gmail.com>
>>
>> Query the min dimensions even if the check
>>     SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
>> fails, but we know that the firmware version is safe. Atm. this is
>> version 8.1 only.
>>
>> With that we don't need quirks for post-2013 models anymore as they
>> expose correct min and max dimensions.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>> ---
>>  drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> index 7a78d75..37d4dff 100644
>> --- a/drivers/input/mouse/synaptics.c
>> +++ b/drivers/input/mouse/synaptics.c
>> @@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
>>         return -1;
>>  }
>>
>> +struct fw_version {
>> +       unsigned char major;
>> +       unsigned char minor;
>> +};
>> +
>> +static const struct fw_version safe_fw_list[] = {
>> +       {8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
>> +       { }
>> +};
>> +
>> +static bool synaptics_is_safe_fw(struct psmouse *psmouse)
>> +{
>> +       struct synaptics_data *priv = psmouse->private;
>> +       int i;
>> +
>> +       for (i = 0; safe_fw_list[i].major; i++) {
>> +               if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
>> +                   safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
>> +                       return true;
>> +       }
>> +
>> +       return false;
>> +}
> 
> This seems a little bit too much for just one firmware ID.
> Plus the name safe_fw_list gives hope that this list can be used for
> something else later, but then it may conflict with it current
> purpose.
> 
> How about we just rely on the current list of PNPIDs we already have
> for this generation?
> This might not be a good idea either, so an other solution would be to
> directly check for firmware 8.1 in the test below.
> 
> I'd like to hear other opinions on this however before we commit to a decision.

I think that checking the firmware version, rather then relying on pnp-ids, is
the right thing to do, this e.g. also gives us more accurate min/max values
on the x230 / t430 series.

As for using the list, rather then just the one id, I'm fine either way, the list
is more future proof, which is why I acked this patch as is. But we could go
with just a check for the single version now and extend this later if needed.

Regards,

Hans


> 
> 
> Cheers,
> Benjamin
> 
> PS: I think Hans already mentioned, but if you want your patches to
> land in Dmitry's tree, it's better to CC him directly when submitting
> a patch.
> 
>> +
>>  /*
>>   * Read touchpad resolution and maximum reported coordinates
>>   * Resolution is left zero if touchpad does not support the query
>> @@ -368,7 +392,8 @@ static int synaptics_resolution(struct psmouse *psmouse)
>>                 }
>>         }
>>
>> -       if (SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 &&
>> +       if ((SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7 ||
>> +            synaptics_is_safe_fw(psmouse)) &&
>>             SYN_CAP_MIN_DIMENSIONS(priv->ext_cap_0c)) {
>>                 if (synaptics_send_cmd(psmouse, SYN_QUE_EXT_MIN_COORDS, resp)) {
>>                         psmouse_warn(psmouse,
>> --
>> 2.2.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Hans de Goede]

Hi,

On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> Hi Daniel,
>>
>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>> <daniel.martin@secunet.com> wrote:
>>> From: Daniel Martin <consume.noise@gmail.com>
>>>
>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>> have post-2013 model and don't need to apply any quirk.
>>>
>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>>> ---
>>>  drivers/input/mouse/synaptics.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>> index 37d4dff..f6c43ff 100644
>>> --- a/drivers/input/mouse/synaptics.c
>>> +++ b/drivers/input/mouse/synaptics.c
>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>         struct synaptics_data *priv = psmouse->private;
>>>         int i;
>>>
>>> +       /* Post-2013 models expose correct dimensions. */
>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>> +               return 0;
>>> +
>>
>> Well, this one, I don't like it either :(
>>
>> At least, the test should be within the psmouse_matches_pnp_id() below
>> to ensure we are deciding with Lenovo devices only.
>>
>> The other concern is hardcoding these values in the code directly.
>> What if Synaptics/Lenovo decides to ship a new released model with
>> proper min_max ranges but with a different offset?
>>
>> Andrew told us that the board ID should be enough to discriminate old
>> and faulty touchpads from the new and valid touchpads.
>>
>> My concern here is that we will have to backport these changes in the
>> various stable kernel and the various distributions. And if we do not
>> end up with the right solution right now, that means that we will have
>> to do the job over and over.
>>
>> I am quite tempted to find a solution in the userspace for that fix.
>> Not sure I'll be able to find the right one right now, but it may
>> worth trying.
>>
> 
> So, the user space solution seems difficult because we do not export
> either the board_id or the firmware_id. So that would required to
> update the kernel anyway, a bunch of user space tools and a hwdb... :(
> 
> How about we just add an extra min/max in struct min_max_quirk,
> compare the current min/max with the 2 possible values and if there is
> a match, we do not override the values.
> This way, we keep the crap of wrong/correct min max in the small list
> of device we know are problematic, and if the new batch of E540 has a
> different correct min/max range, then we will be able to adjust it
> without breaking the other we fixed.
> 
> Dmitry, Hans, any comments on this?

I'm thinking more along the lines of adding a max_broken_board_id field
to the quirks, and if the touchpad board_id is larger then the
max_broken_board_id not use the quirk.

Regards,

Hans

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Benjamin Tissoires]

On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>> <benjamin.tissoires@gmail.com> wrote:
>>> Hi Daniel,
>>>
>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>> <daniel.martin@secunet.com> wrote:
>>>> From: Daniel Martin <consume.noise@gmail.com>
>>>>
>>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>>> have post-2013 model and don't need to apply any quirk.
>>>>
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>>>> ---
>>>>  drivers/input/mouse/synaptics.c | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>> index 37d4dff..f6c43ff 100644
>>>> --- a/drivers/input/mouse/synaptics.c
>>>> +++ b/drivers/input/mouse/synaptics.c
>>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>>         struct synaptics_data *priv = psmouse->private;
>>>>         int i;
>>>>
>>>> +       /* Post-2013 models expose correct dimensions. */
>>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>>> +               return 0;
>>>> +
>>>
>>> Well, this one, I don't like it either :(
>>>
>>> At least, the test should be within the psmouse_matches_pnp_id() below
>>> to ensure we are deciding with Lenovo devices only.
>>>
>>> The other concern is hardcoding these values in the code directly.
>>> What if Synaptics/Lenovo decides to ship a new released model with
>>> proper min_max ranges but with a different offset?
>>>
>>> Andrew told us that the board ID should be enough to discriminate old
>>> and faulty touchpads from the new and valid touchpads.
>>>
>>> My concern here is that we will have to backport these changes in the
>>> various stable kernel and the various distributions. And if we do not
>>> end up with the right solution right now, that means that we will have
>>> to do the job over and over.
>>>
>>> I am quite tempted to find a solution in the userspace for that fix.
>>> Not sure I'll be able to find the right one right now, but it may
>>> worth trying.
>>>
>>
>> So, the user space solution seems difficult because we do not export
>> either the board_id or the firmware_id. So that would required to
>> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>>
>> How about we just add an extra min/max in struct min_max_quirk,
>> compare the current min/max with the 2 possible values and if there is
>> a match, we do not override the values.
>> This way, we keep the crap of wrong/correct min max in the small list
>> of device we know are problematic, and if the new batch of E540 has a
>> different correct min/max range, then we will be able to adjust it
>> without breaking the other we fixed.
>>
>> Dmitry, Hans, any comments on this?
>
> I'm thinking more along the lines of adding a max_broken_board_id field
> to the quirks, and if the touchpad board_id is larger then the
> max_broken_board_id not use the quirk.
>

Yep, this was confirmed by Synaptics that the board id will be
incremented only at each new board revision. So it should be safe to
only check for that.

Cheers,
Benjamin

Re: [PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Benjamin Tissoires]

On Fri, Jan 30, 2015 at 4:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 01/29/2015 07:48 PM, Benjamin Tissoires wrote:
>> Hi Daniel,
>>
>> in one hand I would like to see that upstream ASAP. But on the other
>> hand, I have some concerns here and there regarding this series.
>>
>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>> <daniel.martin@secunet.com> wrote:
>>> From: Daniel Martin <consume.noise@gmail.com>
>>>
>>> Query the min dimensions even if the check
>>>     SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
>>> fails, but we know that the firmware version is safe. Atm. this is
>>> version 8.1 only.
>>>
>>> With that we don't need quirks for post-2013 models anymore as they
>>> expose correct min and max dimensions.
>>>
>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>>> ---
>>>  drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
>>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>> index 7a78d75..37d4dff 100644
>>> --- a/drivers/input/mouse/synaptics.c
>>> +++ b/drivers/input/mouse/synaptics.c
>>> @@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
>>>         return -1;
>>>  }
>>>
>>> +struct fw_version {
>>> +       unsigned char major;
>>> +       unsigned char minor;
>>> +};
>>> +
>>> +static const struct fw_version safe_fw_list[] = {
>>> +       {8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
>>> +       { }
>>> +};
>>> +
>>> +static bool synaptics_is_safe_fw(struct psmouse *psmouse)
>>> +{
>>> +       struct synaptics_data *priv = psmouse->private;
>>> +       int i;
>>> +
>>> +       for (i = 0; safe_fw_list[i].major; i++) {
>>> +               if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
>>> +                   safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
>>> +                       return true;
>>> +       }
>>> +
>>> +       return false;
>>> +}
>>
>> This seems a little bit too much for just one firmware ID.
>> Plus the name safe_fw_list gives hope that this list can be used for
>> something else later, but then it may conflict with it current
>> purpose.
>>
>> How about we just rely on the current list of PNPIDs we already have
>> for this generation?
>> This might not be a good idea either, so an other solution would be to
>> directly check for firmware 8.1 in the test below.
>>
>> I'd like to hear other opinions on this however before we commit to a decision.
>
> I think that checking the firmware version, rather then relying on pnp-ids, is
> the right thing to do, this e.g. also gives us more accurate min/max values
> on the x230 / t430 series.
>
> As for using the list, rather then just the one id, I'm fine either way, the list
> is more future proof, which is why I acked this patch as is. But we could go
> with just a check for the single version now and extend this later if needed.
>

If you are fine either way, I would lean to have only one check. I am
not confident in the "future proof" way of having a list, because the
list is too tight to the current bug. But I think I already made my
point clear enough.
Also, we already reported this to Synaptics, and I think (hope) they
will fix this in their future FW.

Cheers,
Benjamin

Re: [PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Andrew Duggan]

On 01/30/2015 07:41 AM, Benjamin Tissoires wrote:
> On Fri, Jan 30, 2015 at 4:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 01/29/2015 07:48 PM, Benjamin Tissoires wrote:
>>> Hi Daniel,
>>>
>>> in one hand I would like to see that upstream ASAP. But on the other
>>> hand, I have some concerns here and there regarding this series.
>>>
>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>> <daniel.martin@secunet.com> wrote:
>>>> From: Daniel Martin <consume.noise@gmail.com>
>>>>
>>>> Query the min dimensions even if the check
>>>>      SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
>>>> fails, but we know that the firmware version is safe. Atm. this is
>>>> version 8.1 only.
>>>>
>>>> With that we don't need quirks for post-2013 models anymore as they
>>>> expose correct min and max dimensions.
>>>>
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>>>> ---
>>>>   drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
>>>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>> index 7a78d75..37d4dff 100644
>>>> --- a/drivers/input/mouse/synaptics.c
>>>> +++ b/drivers/input/mouse/synaptics.c
>>>> @@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
>>>>          return -1;
>>>>   }
>>>>
>>>> +struct fw_version {
>>>> +       unsigned char major;
>>>> +       unsigned char minor;
>>>> +};
>>>> +
>>>> +static const struct fw_version safe_fw_list[] = {
>>>> +       {8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
>>>> +       { }
>>>> +};
>>>> +
>>>> +static bool synaptics_is_safe_fw(struct psmouse *psmouse)
>>>> +{
>>>> +       struct synaptics_data *priv = psmouse->private;
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; safe_fw_list[i].major; i++) {
>>>> +               if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
>>>> +                   safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
>>>> +                       return true;
>>>> +       }
>>>> +
>>>> +       return false;
>>>> +}
>>> This seems a little bit too much for just one firmware ID.
>>> Plus the name safe_fw_list gives hope that this list can be used for
>>> something else later, but then it may conflict with it current
>>> purpose.
>>>
>>> How about we just rely on the current list of PNPIDs we already have
>>> for this generation?
>>> This might not be a good idea either, so an other solution would be to
>>> directly check for firmware 8.1 in the test below.
>>>
>>> I'd like to hear other opinions on this however before we commit to a decision.
>> I think that checking the firmware version, rather then relying on pnp-ids, is
>> the right thing to do, this e.g. also gives us more accurate min/max values
>> on the x230 / t430 series.
>>
>> As for using the list, rather then just the one id, I'm fine either way, the list
>> is more future proof, which is why I acked this patch as is. But we could go
>> with just a check for the single version now and extend this later if needed.
>>
> If you are fine either way, I would lean to have only one check. I am
> not confident in the "future proof" way of having a list, because the
> list is too tight to the current bug. But I think I already made my
> point clear enough.
> Also, we already reported this to Synaptics, and I think (hope) they
> will fix this in their future FW.

What about just checking it the Reports Min bit is set and if it is 
assuming that the minimum coordinates query exists? The Reports Min bit 
should be reliable even if the number of extended queries above 5 is 
not. Or if that is too broad I would suggest checking if the firmware 
revision major is >= 8. I don't think a list is necessary.

Since it looks like this is still an issue on the 2014 systems I will 
file a firmware bug.

Andrew

>
> Cheers,
> Benjamin

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Andrew Duggan]

On 01/30/2015 07:34 AM, Benjamin Tissoires wrote:
> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>>> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>>> <benjamin.tissoires@gmail.com> wrote:
>>>> Hi Daniel,
>>>>
>>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>>> <daniel.martin@secunet.com> wrote:
>>>>> From: Daniel Martin <consume.noise@gmail.com>
>>>>>
>>>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>>>> have post-2013 model and don't need to apply any quirk.
>>>>>
>>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>>>>> ---
>>>>>   drivers/input/mouse/synaptics.c | 5 +++++
>>>>>   1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>>> index 37d4dff..f6c43ff 100644
>>>>> --- a/drivers/input/mouse/synaptics.c
>>>>> +++ b/drivers/input/mouse/synaptics.c
>>>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>>>          struct synaptics_data *priv = psmouse->private;
>>>>>          int i;
>>>>>
>>>>> +       /* Post-2013 models expose correct dimensions. */
>>>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>>>> +               return 0;
>>>>> +
>>>> Well, this one, I don't like it either :(
>>>>
>>>> At least, the test should be within the psmouse_matches_pnp_id() below
>>>> to ensure we are deciding with Lenovo devices only.
>>>>
>>>> The other concern is hardcoding these values in the code directly.
>>>> What if Synaptics/Lenovo decides to ship a new released model with
>>>> proper min_max ranges but with a different offset?
>>>>
>>>> Andrew told us that the board ID should be enough to discriminate old
>>>> and faulty touchpads from the new and valid touchpads.
>>>>
>>>> My concern here is that we will have to backport these changes in the
>>>> various stable kernel and the various distributions. And if we do not
>>>> end up with the right solution right now, that means that we will have
>>>> to do the job over and over.
>>>>
>>>> I am quite tempted to find a solution in the userspace for that fix.
>>>> Not sure I'll be able to find the right one right now, but it may
>>>> worth trying.
>>>>
>>> So, the user space solution seems difficult because we do not export
>>> either the board_id or the firmware_id. So that would required to
>>> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>>>
>>> How about we just add an extra min/max in struct min_max_quirk,
>>> compare the current min/max with the 2 possible values and if there is
>>> a match, we do not override the values.
>>> This way, we keep the crap of wrong/correct min max in the small list
>>> of device we know are problematic, and if the new batch of E540 has a
>>> different correct min/max range, then we will be able to adjust it
>>> without breaking the other we fixed.
>>>
>>> Dmitry, Hans, any comments on this?
>> I'm thinking more along the lines of adding a max_broken_board_id field
>> to the quirks, and if the touchpad board_id is larger then the
>> max_broken_board_id not use the quirk.
>>
> Yep, this was confirmed by Synaptics that the board id will be
> incremented only at each new board revision. So it should be safe to
> only check for that.

It is worth noting that the format of the modes query changed in 
firmware revision 7.5. If you are going to use board ids you should 
probably check to make sure they have firmware which is 7.5 or later 
before doing the check.

Andrew

> Cheers,
> Benjamin

Re: [PATCH v2 3/5] Input: synaptics - Query min dimensions when safe firmware

[Daniel Martin]

On Fri, Jan 30, 2015 at 02:35:07PM -0800, Andrew Duggan wrote:
> On 01/30/2015 07:41 AM, Benjamin Tissoires wrote:
> >On Fri, Jan 30, 2015 at 4:03 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>Hi,
> >>
> >>On 01/29/2015 07:48 PM, Benjamin Tissoires wrote:
> >>>Hi Daniel,
> >>>
> >>>in one hand I would like to see that upstream ASAP. But on the other
> >>>hand, I have some concerns here and there regarding this series.
> >>>
> >>>On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
> >>><daniel.martin@secunet.com> wrote:
> >>>>From: Daniel Martin <consume.noise@gmail.com>
> >>>>
> >>>>Query the min dimensions even if the check
> >>>>     SYN_EXT_CAP_REQUESTS(priv->capabilities) >= 7
> >>>>fails, but we know that the firmware version is safe. Atm. this is
> >>>>version 8.1 only.
> >>>>
> >>>>With that we don't need quirks for post-2013 models anymore as they
> >>>>expose correct min and max dimensions.
> >>>>
> >>>>Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> >>>>Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> >>>>---
> >>>>  drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
> >>>>  1 file changed, 26 insertions(+), 1 deletion(-)
> >>>>
> >>>>diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >>>>index 7a78d75..37d4dff 100644
> >>>>--- a/drivers/input/mouse/synaptics.c
> >>>>+++ b/drivers/input/mouse/synaptics.c
> >>>>@@ -333,6 +333,30 @@ static int synaptics_identify(struct psmouse *psmouse)
> >>>>         return -1;
> >>>>  }
> >>>>
> >>>>+struct fw_version {
> >>>>+       unsigned char major;
> >>>>+       unsigned char minor;
> >>>>+};
> >>>>+
> >>>>+static const struct fw_version safe_fw_list[] = {
> >>>>+       {8, 1}, /* safe for SYN_QUE_EXT_MIN_COORDS */
> >>>>+       { }
> >>>>+};
> >>>>+
> >>>>+static bool synaptics_is_safe_fw(struct psmouse *psmouse)
> >>>>+{
> >>>>+       struct synaptics_data *priv = psmouse->private;
> >>>>+       int i;
> >>>>+
> >>>>+       for (i = 0; safe_fw_list[i].major; i++) {
> >>>>+               if (safe_fw_list[i].major == SYN_ID_MAJOR(priv->identity) &&
> >>>>+                   safe_fw_list[i].minor == SYN_ID_MINOR(priv->identity))
> >>>>+                       return true;
> >>>>+       }
> >>>>+
> >>>>+       return false;
> >>>>+}
> >>>This seems a little bit too much for just one firmware ID.
> >>>Plus the name safe_fw_list gives hope that this list can be used for
> >>>something else later, but then it may conflict with it current
> >>>purpose.
> >>>
> >>>How about we just rely on the current list of PNPIDs we already have
> >>>for this generation?
> >>>This might not be a good idea either, so an other solution would be to
> >>>directly check for firmware 8.1 in the test below.
> >>>
> >>>I'd like to hear other opinions on this however before we commit to a decision.
> >>I think that checking the firmware version, rather then relying on pnp-ids, is
> >>the right thing to do, this e.g. also gives us more accurate min/max values
> >>on the x230 / t430 series.
> >>
> >>As for using the list, rather then just the one id, I'm fine either way, the list
> >>is more future proof, which is why I acked this patch as is. But we could go
> >>with just a check for the single version now and extend this later if needed.
> >>
> >If you are fine either way, I would lean to have only one check. I am
> >not confident in the "future proof" way of having a list, because the
> >list is too tight to the current bug. But I think I already made my
> >point clear enough.
> >Also, we already reported this to Synaptics, and I think (hope) they
> >will fix this in their future FW.
> 
> What about just checking it the Reports Min bit is set and if it is assuming
> that the minimum coordinates query exists? The Reports Min bit should be
> reliable even if the number of extended queries above 5 is not. Or if that
> is too broad I would suggest checking if the firmware revision major is >=
> 8. I don't think a list is necessary.

I had this idea too. Then Hans pointed out that there are (older)
buggy firmwares having the min bit set, but not supporting the query and
sending an unsupported query might crash the firmware.

> Since it looks like this is still an issue on the 2014 systems I will file a
> firmware bug.
> 
> Andrew
> 
> >
> >Cheers,
> >Benjamin
> 

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Daniel Martin]

On Fri, Jan 30, 2015 at 10:34:22AM -0500, Benjamin Tissoires wrote:
> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi,
> >
> > On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
> >> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
> >> <benjamin.tissoires@gmail.com> wrote:
> >>> Hi Daniel,
> >>>
> >>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
> >>> <daniel.martin@secunet.com> wrote:
> >>>> From: Daniel Martin <consume.noise@gmail.com>
> >>>>
> >>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
> >>>> have post-2013 model and don't need to apply any quirk.
> >>>>
> >>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
> >>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
> >>>> ---
> >>>>  drivers/input/mouse/synaptics.c | 5 +++++
> >>>>  1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >>>> index 37d4dff..f6c43ff 100644
> >>>> --- a/drivers/input/mouse/synaptics.c
> >>>> +++ b/drivers/input/mouse/synaptics.c
> >>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
> >>>>         struct synaptics_data *priv = psmouse->private;
> >>>>         int i;
> >>>>
> >>>> +       /* Post-2013 models expose correct dimensions. */
> >>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
> >>>> +           priv->y_min == 1170 && priv->y_max == 4684)
> >>>> +               return 0;
> >>>> +
> >>>
> >>> Well, this one, I don't like it either :(
> >>>
> >>> At least, the test should be within the psmouse_matches_pnp_id() below
> >>> to ensure we are deciding with Lenovo devices only.
> >>>
> >>> The other concern is hardcoding these values in the code directly.
> >>> What if Synaptics/Lenovo decides to ship a new released model with
> >>> proper min_max ranges but with a different offset?
> >>>
> >>> Andrew told us that the board ID should be enough to discriminate old
> >>> and faulty touchpads from the new and valid touchpads.
> >>>
> >>> My concern here is that we will have to backport these changes in the
> >>> various stable kernel and the various distributions. And if we do not
> >>> end up with the right solution right now, that means that we will have
> >>> to do the job over and over.
> >>>
> >>> I am quite tempted to find a solution in the userspace for that fix.
> >>> Not sure I'll be able to find the right one right now, but it may
> >>> worth trying.
> >>>
> >>
> >> So, the user space solution seems difficult because we do not export
> >> either the board_id or the firmware_id. So that would required to
> >> update the kernel anyway, a bunch of user space tools and a hwdb... :(
> >>
> >> How about we just add an extra min/max in struct min_max_quirk,
> >> compare the current min/max with the 2 possible values and if there is
> >> a match, we do not override the values.
> >> This way, we keep the crap of wrong/correct min max in the small list
> >> of device we know are problematic, and if the new batch of E540 has a
> >> different correct min/max range, then we will be able to adjust it
> >> without breaking the other we fixed.
> >>
> >> Dmitry, Hans, any comments on this?
> >
> > I'm thinking more along the lines of adding a max_broken_board_id field
> > to the quirks, and if the touchpad board_id is larger then the
> > max_broken_board_id not use the quirk.
> >
> 
> Yep, this was confirmed by Synaptics that the board id will be
> incremented only at each new board revision. So it should be safe to
> only check for that.

Could you ask your contact for an exact board id, since when the ranges
have been fixed? From the data I can look at it seems to be <= 2962.

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Benjamin Tissoires]

On Sat, Jan 31, 2015 at 3:18 AM, Daniel Martin <consume.noise@gmail.com> wrote:
> On Fri, Jan 30, 2015 at 10:34:22AM -0500, Benjamin Tissoires wrote:
>> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> > Hi,
>> >
>> > On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>> >> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>> >> <benjamin.tissoires@gmail.com> wrote:
>> >>> Hi Daniel,
>> >>>
>> >>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>> >>> <daniel.martin@secunet.com> wrote:
>> >>>> From: Daniel Martin <consume.noise@gmail.com>
>> >>>>
>> >>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>> >>>> have post-2013 model and don't need to apply any quirk.
>> >>>>
>> >>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>> >>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>> >>>> ---
>> >>>>  drivers/input/mouse/synaptics.c | 5 +++++
>> >>>>  1 file changed, 5 insertions(+)
>> >>>>
>> >>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>> >>>> index 37d4dff..f6c43ff 100644
>> >>>> --- a/drivers/input/mouse/synaptics.c
>> >>>> +++ b/drivers/input/mouse/synaptics.c
>> >>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>> >>>>         struct synaptics_data *priv = psmouse->private;
>> >>>>         int i;
>> >>>>
>> >>>> +       /* Post-2013 models expose correct dimensions. */
>> >>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>> >>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>> >>>> +               return 0;
>> >>>> +
>> >>>
>> >>> Well, this one, I don't like it either :(
>> >>>
>> >>> At least, the test should be within the psmouse_matches_pnp_id() below
>> >>> to ensure we are deciding with Lenovo devices only.
>> >>>
>> >>> The other concern is hardcoding these values in the code directly.
>> >>> What if Synaptics/Lenovo decides to ship a new released model with
>> >>> proper min_max ranges but with a different offset?
>> >>>
>> >>> Andrew told us that the board ID should be enough to discriminate old
>> >>> and faulty touchpads from the new and valid touchpads.
>> >>>
>> >>> My concern here is that we will have to backport these changes in the
>> >>> various stable kernel and the various distributions. And if we do not
>> >>> end up with the right solution right now, that means that we will have
>> >>> to do the job over and over.
>> >>>
>> >>> I am quite tempted to find a solution in the userspace for that fix.
>> >>> Not sure I'll be able to find the right one right now, but it may
>> >>> worth trying.
>> >>>
>> >>
>> >> So, the user space solution seems difficult because we do not export
>> >> either the board_id or the firmware_id. So that would required to
>> >> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>> >>
>> >> How about we just add an extra min/max in struct min_max_quirk,
>> >> compare the current min/max with the 2 possible values and if there is
>> >> a match, we do not override the values.
>> >> This way, we keep the crap of wrong/correct min max in the small list
>> >> of device we know are problematic, and if the new batch of E540 has a
>> >> different correct min/max range, then we will be able to adjust it
>> >> without breaking the other we fixed.
>> >>
>> >> Dmitry, Hans, any comments on this?
>> >
>> > I'm thinking more along the lines of adding a max_broken_board_id field
>> > to the quirks, and if the touchpad board_id is larger then the
>> > max_broken_board_id not use the quirk.
>> >
>>
>> Yep, this was confirmed by Synaptics that the board id will be
>> incremented only at each new board revision. So it should be safe to
>> only check for that.
>
> Could you ask your contact for an exact board id, since when the ranges
> have been fixed? From the data I can look at it seems to be <= 2962.

IIRC, Andrew said in a private mail that extracting this kind of
information is quite tricky.

I would say that we should add a per-pnp_id board limit with the data we know.

I think you could add this in the struct min_max_quirk, and if the
max_board_id is > 0, we check against it.
This way, we could have a finer grain when dealing with the hardware refreshes.

Cheers,
Benjamin

Re: [PATCH v2 4/5] Input: synaptics - Skip quirks when post-2013 dimensions

[Hans de Goede]

Hi,

On 02/05/2015 10:22 PM, Benjamin Tissoires wrote:
> On Sat, Jan 31, 2015 at 3:18 AM, Daniel Martin <consume.noise@gmail.com> wrote:
>> On Fri, Jan 30, 2015 at 10:34:22AM -0500, Benjamin Tissoires wrote:
>>> On Fri, Jan 30, 2015 at 4:59 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>> On 01/29/2015 08:50 PM, Benjamin Tissoires wrote:
>>>>> On Thu, Jan 29, 2015 at 2:02 PM, Benjamin Tissoires
>>>>> <benjamin.tissoires@gmail.com> wrote:
>>>>>> Hi Daniel,
>>>>>>
>>>>>> On Tue, Jan 27, 2015 at 3:34 AM, Daniel Martin
>>>>>> <daniel.martin@secunet.com> wrote:
>>>>>>> From: Daniel Martin <consume.noise@gmail.com>
>>>>>>>
>>>>>>> If we queried min/max dimensions of x [1266..5674], y [1170..4684] we
>>>>>>> have post-2013 model and don't need to apply any quirk.
>>>>>>>
>>>>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=91541
>>>>>>> Signed-off-by: Daniel Martin <consume.noise@gmail.com>
>>>>>>> ---
>>>>>>>  drivers/input/mouse/synaptics.c | 5 +++++
>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
>>>>>>> index 37d4dff..f6c43ff 100644
>>>>>>> --- a/drivers/input/mouse/synaptics.c
>>>>>>> +++ b/drivers/input/mouse/synaptics.c
>>>>>>> @@ -420,6 +420,11 @@ static int synaptics_quirks(struct psmouse *psmouse)
>>>>>>>         struct synaptics_data *priv = psmouse->private;
>>>>>>>         int i;
>>>>>>>
>>>>>>> +       /* Post-2013 models expose correct dimensions. */
>>>>>>> +       if (priv->x_min == 1266 && priv->x_max == 5674 &&
>>>>>>> +           priv->y_min == 1170 && priv->y_max == 4684)
>>>>>>> +               return 0;
>>>>>>> +
>>>>>>
>>>>>> Well, this one, I don't like it either :(
>>>>>>
>>>>>> At least, the test should be within the psmouse_matches_pnp_id() below
>>>>>> to ensure we are deciding with Lenovo devices only.
>>>>>>
>>>>>> The other concern is hardcoding these values in the code directly.
>>>>>> What if Synaptics/Lenovo decides to ship a new released model with
>>>>>> proper min_max ranges but with a different offset?
>>>>>>
>>>>>> Andrew told us that the board ID should be enough to discriminate old
>>>>>> and faulty touchpads from the new and valid touchpads.
>>>>>>
>>>>>> My concern here is that we will have to backport these changes in the
>>>>>> various stable kernel and the various distributions. And if we do not
>>>>>> end up with the right solution right now, that means that we will have
>>>>>> to do the job over and over.
>>>>>>
>>>>>> I am quite tempted to find a solution in the userspace for that fix.
>>>>>> Not sure I'll be able to find the right one right now, but it may
>>>>>> worth trying.
>>>>>>
>>>>>
>>>>> So, the user space solution seems difficult because we do not export
>>>>> either the board_id or the firmware_id. So that would required to
>>>>> update the kernel anyway, a bunch of user space tools and a hwdb... :(
>>>>>
>>>>> How about we just add an extra min/max in struct min_max_quirk,
>>>>> compare the current min/max with the 2 possible values and if there is
>>>>> a match, we do not override the values.
>>>>> This way, we keep the crap of wrong/correct min max in the small list
>>>>> of device we know are problematic, and if the new batch of E540 has a
>>>>> different correct min/max range, then we will be able to adjust it
>>>>> without breaking the other we fixed.
>>>>>
>>>>> Dmitry, Hans, any comments on this?
>>>>
>>>> I'm thinking more along the lines of adding a max_broken_board_id field
>>>> to the quirks, and if the touchpad board_id is larger then the
>>>> max_broken_board_id not use the quirk.
>>>>
>>>
>>> Yep, this was confirmed by Synaptics that the board id will be
>>> incremented only at each new board revision. So it should be safe to
>>> only check for that.
>>
>> Could you ask your contact for an exact board id, since when the ranges
>> have been fixed? From the data I can look at it seems to be <= 2962.
> 
> IIRC, Andrew said in a private mail that extracting this kind of
> information is quite tricky.
> 
> I would say that we should add a per-pnp_id board limit with the data we know.
> 
> I think you could add this in the struct min_max_quirk, and if the
> max_board_id is > 0, we check against it.
> This way, we could have a finer grain when dealing with the hardware refreshes.

ACK, I agree that this is the best way forward with this.

Regards,

Hans