diff mbox

[2/3] input: alps: For protocol V3, do not process data when last packet's bit7 is set

Message ID 1412329392-5580-3-git-send-email-pali.rohar@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pali Rohár Oct. 3, 2014, 9:43 a.m. UTC 
Sometimes laptops with closed lid receive invalid ALPS protocol V3 packets with
last bit7 set.

This happens on Dell Latitude laptops and it looks like it is BIOS bug. Probably
EC does not correctly split keyboard and touchpad PS/2 data and when laptop lid
is closed it adds 0xFF packet into touchpad PS/2 data.

When last packet's bit7 is set it is invalid ALPS packet. So Do not process it
and drop it with PSMOUSE_FULL_PACKET.

After testing it looks like this patch fixed problem with reseting mouse (after
series of invalid packets) when laptop lid is closed on Dell Latitude machines.

Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
Tested-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/input/mouse/alps.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Hans de Goede Oct. 3, 2014, 9:51 a.m. UTC | #1 
Hi,

On 10/03/2014 11:43 AM, Pali Rohár wrote:
> Sometimes laptops with closed lid receive invalid ALPS protocol V3 packets with
> last bit7 set.
> 
> This happens on Dell Latitude laptops and it looks like it is BIOS bug. Probably
> EC does not correctly split keyboard and touchpad PS/2 data and when laptop lid
> is closed it adds 0xFF packet into touchpad PS/2 data.
> 
> When last packet's bit7 is set it is invalid ALPS packet. So Do not process it
> and drop it with PSMOUSE_FULL_PACKET.
> 
> After testing it looks like this patch fixed problem with reseting mouse (after
> series of invalid packets) when laptop lid is closed on Dell Latitude machines.
> 
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> Tested-by: Pali Rohár <pali.rohar@gmail.com>
> ---
>  drivers/input/mouse/alps.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 1bd5aa1..b1f44d4 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1179,6 +1179,16 @@ static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
>  		return PSMOUSE_BAD_DATA;
>  	}
>  
> +	if (priv->proto_version == ALPS_PROTO_V3 && psmouse->pktcnt == psmouse->pktsize) {
> +		// For protocol V3, do not process data when last packet's bit7 is set
> +		if (psmouse->packet[psmouse->pktcnt - 1] & 0x80) {
> +			psmouse_dbg(psmouse, "v3 discard packet[%i] = %x\n",
> +				    psmouse->pktcnt - 1,
> +				    psmouse->packet[psmouse->pktcnt - 1]);
> +			return PSMOUSE_FULL_PACKET;
> +		}
> +	}
> +

I wonder if this should not be PSMOUSE_BAD_DATA as a return value ? I realize that
with the 3th patch in place, that will cause an immediate reset, but if your theory is
right that this ff gets inserted from the keyboard ps/2 stream, then we actually want
a full reset as otherwise we will be out of sync then.

>  	/* Bytes 2 - pktsize should have 0 in the highest bit */
>  	if ((priv->proto_version < ALPS_PROTO_V5) &&
>  	    psmouse->pktcnt >= 2 && psmouse->pktcnt <= psmouse->pktsize &&
> 

Regards,

Hans
--
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
Pali Rohár Oct. 3, 2014, 9:58 a.m. UTC | #2 
On Friday 03 October 2014 11:51:22 Hans de Goede wrote:
> Hi,
> 
> On 10/03/2014 11:43 AM, Pali Rohár wrote:
> > Sometimes laptops with closed lid receive invalid ALPS
> > protocol V3 packets with last bit7 set.
> > 
> > This happens on Dell Latitude laptops and it looks like it
> > is BIOS bug. Probably EC does not correctly split keyboard
> > and touchpad PS/2 data and when laptop lid is closed it
> > adds 0xFF packet into touchpad PS/2 data.
> > 
> > When last packet's bit7 is set it is invalid ALPS packet. So
> > Do not process it and drop it with PSMOUSE_FULL_PACKET.
> > 
> > After testing it looks like this patch fixed problem with
> > reseting mouse (after series of invalid packets) when
> > laptop lid is closed on Dell Latitude machines.
> > 
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > Tested-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/input/mouse/alps.c |   10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/input/mouse/alps.c
> > b/drivers/input/mouse/alps.c index 1bd5aa1..b1f44d4 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -1179,6 +1179,16 @@ static psmouse_ret_t
> > alps_process_byte(struct psmouse *psmouse)
> > 
> >  		return PSMOUSE_BAD_DATA;
> >  	
> >  	}
> > 
> > +	if (priv->proto_version == ALPS_PROTO_V3 &&
> > psmouse->pktcnt == psmouse->pktsize) { +		// For protocol
> > V3, do not process data when last packet's bit7 is set
> > +		if (psmouse->packet[psmouse->pktcnt - 1] & 0x80) {
> > +			psmouse_dbg(psmouse, "v3 discard packet[%i] = 
%x\n",
> > +				    psmouse->pktcnt - 1,
> > +				    psmouse->packet[psmouse->pktcnt - 1]);
> > +			return PSMOUSE_FULL_PACKET;
> > +		}
> > +	}
> > +
> 
> I wonder if this should not be PSMOUSE_BAD_DATA as a return
> value ? I realize that with the 3th patch in place, that will
> cause an immediate reset, but if your theory is right that
> this ff gets inserted from the keyboard ps/2 stream, then we
> actually want a full reset as otherwise we will be out of
> sync then.
> 

We really do not know what is root cause of this problem. 
Probably it is BIOS/EC/firmware/... but it could be something 
other too.

On my laptop - Dell Latitude E6440 - I'm getting these invalid 
packets when I close LID. And they are generated too many (lot of 
per second). When PSMOUSE_FULL_PACKET is returned then no driver 
does not reset touchpad. With PSMOUSE_BAD_DATA psmouse decide 
that it needs reset and with 3rd patch psmouse could try to reset 
device too many times... So I think PSMOUSE_FULL_PACKET is better 
here. And having dmesg log full of device resets is probably not 
good too when laptop lid is closed.

> >  	/* Bytes 2 - pktsize should have 0 in the highest bit */
> >  	if ((priv->proto_version < ALPS_PROTO_V5) &&
> >  	
> >  	    psmouse->pktcnt >= 2 && psmouse->pktcnt <=
> >  	    psmouse->pktsize &&
> 
> Regards,
> 
> Hans
Hans de Goede Oct. 3, 2014, 10:01 a.m. UTC | #3 
Hi,

On 10/03/2014 11:58 AM, Pali Rohár wrote:
> On Friday 03 October 2014 11:51:22 Hans de Goede wrote:
>> Hi,
>>
>> On 10/03/2014 11:43 AM, Pali Rohár wrote:
>>> Sometimes laptops with closed lid receive invalid ALPS
>>> protocol V3 packets with last bit7 set.
>>>
>>> This happens on Dell Latitude laptops and it looks like it
>>> is BIOS bug. Probably EC does not correctly split keyboard
>>> and touchpad PS/2 data and when laptop lid is closed it
>>> adds 0xFF packet into touchpad PS/2 data.
>>>
>>> When last packet's bit7 is set it is invalid ALPS packet. So
>>> Do not process it and drop it with PSMOUSE_FULL_PACKET.
>>>
>>> After testing it looks like this patch fixed problem with
>>> reseting mouse (after series of invalid packets) when
>>> laptop lid is closed on Dell Latitude machines.
>>>
>>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>>> Tested-by: Pali Rohár <pali.rohar@gmail.com>
>>> ---
>>>
>>>  drivers/input/mouse/alps.c |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/input/mouse/alps.c
>>> b/drivers/input/mouse/alps.c index 1bd5aa1..b1f44d4 100644
>>> --- a/drivers/input/mouse/alps.c
>>> +++ b/drivers/input/mouse/alps.c
>>> @@ -1179,6 +1179,16 @@ static psmouse_ret_t
>>> alps_process_byte(struct psmouse *psmouse)
>>>
>>>  		return PSMOUSE_BAD_DATA;
>>>  	
>>>  	}
>>>
>>> +	if (priv->proto_version == ALPS_PROTO_V3 &&
>>> psmouse->pktcnt == psmouse->pktsize) { +		// For protocol
>>> V3, do not process data when last packet's bit7 is set
>>> +		if (psmouse->packet[psmouse->pktcnt - 1] & 0x80) {
>>> +			psmouse_dbg(psmouse, "v3 discard packet[%i] = 
> %x\n",
>>> +				    psmouse->pktcnt - 1,
>>> +				    psmouse->packet[psmouse->pktcnt - 1]);
>>> +			return PSMOUSE_FULL_PACKET;
>>> +		}
>>> +	}
>>> +
>>
>> I wonder if this should not be PSMOUSE_BAD_DATA as a return
>> value ? I realize that with the 3th patch in place, that will
>> cause an immediate reset, but if your theory is right that
>> this ff gets inserted from the keyboard ps/2 stream, then we
>> actually want a full reset as otherwise we will be out of
>> sync then.
>>
> 
> We really do not know what is root cause of this problem. 
> Probably it is BIOS/EC/firmware/... but it could be something 
> other too.
> 
> On my laptop - Dell Latitude E6440 - I'm getting these invalid 
> packets when I close LID. And they are generated too many (lot of 
> per second). When PSMOUSE_FULL_PACKET is returned then no driver 
> does not reset touchpad. With PSMOUSE_BAD_DATA psmouse decide 
> that it needs reset and with 3rd patch psmouse could try to reset 
> device too many times... So I think PSMOUSE_FULL_PACKET is better 
> here. And having dmesg log full of device resets is probably not 
> good too when laptop lid is closed.

Ok, then lets stick with PSMOUSE_FULL_PACKET.

So this patch also is:

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

Can you please do a v2, with the following tags added to the
commit messages ? :

- Cc: stable@vger.kernel.org
- Acked-by: Hans de Goede <hdegoede@redhat.com>
- Bug: ... for the relevant bugs

Thanks & Regards,

Hans
--
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
diff mbox

Patch

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 1bd5aa1..b1f44d4 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1179,6 +1179,16 @@  static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
 		return PSMOUSE_BAD_DATA;
 	}
 
+	if (priv->proto_version == ALPS_PROTO_V3 && psmouse->pktcnt == psmouse->pktsize) {
+		// For protocol V3, do not process data when last packet's bit7 is set
+		if (psmouse->packet[psmouse->pktcnt - 1] & 0x80) {
+			psmouse_dbg(psmouse, "v3 discard packet[%i] = %x\n",
+				    psmouse->pktcnt - 1,
+				    psmouse->packet[psmouse->pktcnt - 1]);
+			return PSMOUSE_FULL_PACKET;
+		}
+	}
+
 	/* Bytes 2 - pktsize should have 0 in the highest bit */
 	if ((priv->proto_version < ALPS_PROTO_V5) &&
 	    psmouse->pktcnt >= 2 && psmouse->pktcnt <= psmouse->pktsize &&