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

Commit Message

Daniel Martin Jan. 27, 2015, 8:34 a.m. UTC

From: Daniel Martin <[e-mail]>

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 <[e-mail]>
---
 drivers/input/mouse/synaptics.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Comments

Benjamin Tissoires Jan. 29, 2015, 6:48 p.m. UTC | #1

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
<[e-mail]> wrote:
> From: Daniel Martin <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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 [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hans de Goede Jan. 30, 2015, 9:03 a.m. UTC | #2

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
> <[e-mail]> wrote:
>> From: Daniel Martin <[e-mail]>
>>
>> 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 <[e-mail]>
>> ---
>>  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 [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Benjamin Tissoires Jan. 30, 2015, 3:41 p.m. UTC | #3

On Fri, Jan 30, 2015 at 4:03 AM, Hans de Goede <[e-mail]> 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
>> <[e-mail]> wrote:
>>> From: Daniel Martin <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> ---
>>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Andrew Duggan Jan. 30, 2015, 10:35 p.m. UTC | #4

On 01/30/2015 07:41 AM, Benjamin Tissoires wrote:
> On Fri, Jan 30, 2015 at 4:03 AM, Hans de Goede <[e-mail]> 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
>>> <[e-mail]> wrote:
>>>> From: Daniel Martin <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> ---
>>>>   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

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Martin Jan. 31, 2015, 8:11 a.m. UTC | #5

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 <[e-mail]> 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
> >>><[e-mail]> wrote:
> >>>>From: Daniel Martin <[e-mail]>
> >>>>
> >>>>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 <[e-mail]>
> >>>>---
> >>>>  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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,