[PATCH] Input: Do not add SYN_REPORT in between a single packet data

[PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

As mentioned in documentation, SYN_REPORT should be used to separate two packets
and should not be inserted in between a single packet as otherwise with multiple
SYN_REPORT in a single packet, input reader would not be able to know when the
packet ended really.

Documentation snippet:
* SYN_REPORT:
  - Used to synchronize and separate events into packets of input data changes
    occurring at the same moment in time. For example, motion of a mouse may set
    the REL_X and REL_Y values for one motion, then emit a SYN_REPORT. The next
    motion will emit more REL_X and REL_Y values and send another SYN_REPORT.

Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
 drivers/input/input.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 8806059..262ef77 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
 		if (dev->num_vals >= 2)
 			input_pass_values(dev, dev->vals, dev->num_vals);
 		dev->num_vals = 0;
-	} else if (dev->num_vals >= dev->max_vals - 2) {
-		dev->vals[dev->num_vals++] = input_value_sync;
+	} else if (dev->num_vals >= dev->max_vals - 1) {
 		input_pass_values(dev, dev->vals, dev->num_vals);
 		dev->num_vals = 0;
 	}
-- 
2.6.2

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hello Mr. Torokhov,


Could you kindly help to update about below patch?

Thanks,
Aniroop Mathur


On Mon, Mar 7, 2016 at 11:14 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
> As mentioned in documentation, SYN_REPORT should be used to separate two packets
> and should not be inserted in between a single packet as otherwise with multiple
> SYN_REPORT in a single packet, input reader would not be able to know when the
> packet ended really.
>
> Documentation snippet:
> * SYN_REPORT:
>   - Used to synchronize and separate events into packets of input data changes
>     occurring at the same moment in time. For example, motion of a mouse may set
>     the REL_X and REL_Y values for one motion, then emit a SYN_REPORT. The next
>     motion will emit more REL_X and REL_Y values and send another SYN_REPORT.
>
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
>  drivers/input/input.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 8806059..262ef77 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>                 if (dev->num_vals >= 2)
>                         input_pass_values(dev, dev->vals, dev->num_vals);
>                 dev->num_vals = 0;
> -       } else if (dev->num_vals >= dev->max_vals - 2) {
> -               dev->vals[dev->num_vals++] = input_value_sync;
> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>                 input_pass_values(dev, dev->vals, dev->num_vals);
>                 dev->num_vals = 0;
>         }
> --
> 2.6.2
>

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Dmitry Torokhov]

On Mon, Mar 7, 2016 at 9:44 AM, Aniroop Mathur <a.mathur@samsung.com> wrote:
> As mentioned in documentation, SYN_REPORT should be used to separate two packets
> and should not be inserted in between a single packet as otherwise with multiple
> SYN_REPORT in a single packet, input reader would not be able to know when the
> packet ended really.
>
> Documentation snippet:
> * SYN_REPORT:
>   - Used to synchronize and separate events into packets of input data changes
>     occurring at the same moment in time. For example, motion of a mouse may set
>     the REL_X and REL_Y values for one motion, then emit a SYN_REPORT. The next
>     motion will emit more REL_X and REL_Y values and send another SYN_REPORT.
>
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> ---
>  drivers/input/input.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 8806059..262ef77 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>                 if (dev->num_vals >= 2)
>                         input_pass_values(dev, dev->vals, dev->num_vals);
>                 dev->num_vals = 0;
> -       } else if (dev->num_vals >= dev->max_vals - 2) {
> -               dev->vals[dev->num_vals++] = input_value_sync;
> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>                 input_pass_values(dev, dev->vals, dev->num_vals);
>                 dev->num_vals = 0;
>         }

This makes sense to me. Henrik?

-- 
Dmitry

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Henrik Rydberg]

Hi Dmitry,

>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index 8806059..262ef77 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>>                 if (dev->num_vals >= 2)
>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>>                 dev->num_vals = 0;
>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>> -               dev->vals[dev->num_vals++] = input_value_sync;
>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>>                 dev->num_vals = 0;
>>         }
> 
> This makes sense to me. Henrik?

I went through the commits that made these changes, and I cannot see any strong
reason to keep it. However, this code path only triggers if no SYN events are
seen, as in a driver that fails to emit them and consequently fills up the
buffer. In other words, this change would only affect a device that is already,
to some degree, broken.

So, the question to Aniroop is: do you see this problem in practise, and in that
case, for what driver?

Henrik

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hi Henrik,

On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> Hi Dmitry,
>
>>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>> index 8806059..262ef77 100644
>>> --- a/drivers/input/input.c
>>> +++ b/drivers/input/input.c
>>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>>>                 if (dev->num_vals >= 2)
>>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>>>                 dev->num_vals = 0;
>>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>>> -               dev->vals[dev->num_vals++] = input_value_sync;
>>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>>>                 dev->num_vals = 0;
>>>         }
>>
>> This makes sense to me. Henrik?
>
> I went through the commits that made these changes, and I cannot see any strong
> reason to keep it. However, this code path only triggers if no SYN events are
> seen, as in a driver that fails to emit them and consequently fills up the
> buffer. In other words, this change would only affect a device that is already,
> to some degree, broken.
>
> So, the question to Aniroop is: do you see this problem in practise, and in that
> case, for what driver?
>

Nope. So far I have not dealt with any such driver.
I made this change because it is breaking protocol of SYN_REPORT event code.

Further from the code, I could deduce that max_vals is just an estimation of
packet_size and it does not guarantee that packet_size is same as max_vals.
So real packet_size can be more than max_vals value and hence we could not
insert SYN_REPORT until packet ends really.
Further, if we consider that there exists a driver or will exist in future
which sets capability of x event code according to which max_value comes out to
y and the real packet size is z i.e. driver wants to send same event codes
again in the same packet, so input event reader would be expecting SYN_REPORT
after z events but due to current code SYN_REPORT will get inserted
automatically after y events, which is a wrong behaviour.

Thanks,
Aniroop Mathur

> Henrik
>

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hello Mr. Torokhov,

Could you kindly help to update about this patch?

Thank you,
Aniroop Mathur


On Fri, Mar 11, 2016 at 12:26 AM, Aniroop Mathur
<aniroop.mathur@gmail.com> wrote:
> Hi Henrik,
>
> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>> Hi Dmitry,
>>
>>>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>>> index 8806059..262ef77 100644
>>>> --- a/drivers/input/input.c
>>>> +++ b/drivers/input/input.c
>>>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>>>>                 if (dev->num_vals >= 2)
>>>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>>>>                 dev->num_vals = 0;
>>>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>>>> -               dev->vals[dev->num_vals++] = input_value_sync;
>>>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>>>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>>>>                 dev->num_vals = 0;
>>>>         }
>>>
>>> This makes sense to me. Henrik?
>>
>> I went through the commits that made these changes, and I cannot see any strong
>> reason to keep it. However, this code path only triggers if no SYN events are
>> seen, as in a driver that fails to emit them and consequently fills up the
>> buffer. In other words, this change would only affect a device that is already,
>> to some degree, broken.
>>
>> So, the question to Aniroop is: do you see this problem in practise, and in that
>> case, for what driver?
>>
>
> Nope. So far I have not dealt with any such driver.
> I made this change because it is breaking protocol of SYN_REPORT event code.
>
> Further from the code, I could deduce that max_vals is just an estimation of
> packet_size and it does not guarantee that packet_size is same as max_vals.
> So real packet_size can be more than max_vals value and hence we could not
> insert SYN_REPORT until packet ends really.
> Further, if we consider that there exists a driver or will exist in future
> which sets capability of x event code according to which max_value comes out to
> y and the real packet size is z i.e. driver wants to send same event codes
> again in the same packet, so input event reader would be expecting SYN_REPORT
> after z events but due to current code SYN_REPORT will get inserted
> automatically after y events, which is a wrong behaviour.
>
> Thanks,
> Aniroop Mathur
>
>> Henrik
>>

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hello Mr. Torokhov / Mr. Henry,

On Wed, Mar 16, 2016 at 11:54 PM, Aniroop Mathur
<aniroop.mathur@gmail.com> wrote:
> Hello Mr. Torokhov,
>
> Could you kindly help to update about this patch?
>

So is this patch concluded? Are you applying it?

Thanks,
Aniroop Mathur

> Thank you,
> Aniroop Mathur
>
>
> On Fri, Mar 11, 2016 at 12:26 AM, Aniroop Mathur
> <aniroop.mathur@gmail.com> wrote:
>> Hi Henrik,
>>
>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>>> Hi Dmitry,
>>>
>>>>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>>>> index 8806059..262ef77 100644
>>>>> --- a/drivers/input/input.c
>>>>> +++ b/drivers/input/input.c
>>>>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>>>>>                 if (dev->num_vals >= 2)
>>>>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>>>>>                 dev->num_vals = 0;
>>>>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>>>>> -               dev->vals[dev->num_vals++] = input_value_sync;
>>>>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>>>>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>>>>>                 dev->num_vals = 0;
>>>>>         }
>>>>
>>>> This makes sense to me. Henrik?
>>>
>>> I went through the commits that made these changes, and I cannot see any strong
>>> reason to keep it. However, this code path only triggers if no SYN events are
>>> seen, as in a driver that fails to emit them and consequently fills up the
>>> buffer. In other words, this change would only affect a device that is already,
>>> to some degree, broken.
>>>
>>> So, the question to Aniroop is: do you see this problem in practise, and in that
>>> case, for what driver?
>>>
>>
>> Nope. So far I have not dealt with any such driver.
>> I made this change because it is breaking protocol of SYN_REPORT event code.
>>
>> Further from the code, I could deduce that max_vals is just an estimation of
>> packet_size and it does not guarantee that packet_size is same as max_vals.
>> So real packet_size can be more than max_vals value and hence we could not
>> insert SYN_REPORT until packet ends really.
>> Further, if we consider that there exists a driver or will exist in future
>> which sets capability of x event code according to which max_value comes out to
>> y and the real packet size is z i.e. driver wants to send same event codes
>> again in the same packet, so input event reader would be expecting SYN_REPORT
>> after z events but due to current code SYN_REPORT will get inserted
>> automatically after y events, which is a wrong behaviour.
>>
>> Thanks,
>> Aniroop Mathur
>>
>>> Henrik
>>>

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hello Mr. Henrik,

So do you have any further comments on this patch please?
It is pending for more than 20 days. It would be really appreciating
if you could help out to conclude it as soon as possible.

Regards,
Aniroop Mathur

On Thu, Mar 24, 2016 at 1:05 AM, Aniroop Mathur
<aniroop.mathur@gmail.com> wrote:
> Hello Mr. Torokhov / Mr. Henry,
>
> On Wed, Mar 16, 2016 at 11:54 PM, Aniroop Mathur
> <aniroop.mathur@gmail.com> wrote:
>> Hello Mr. Torokhov,
>>
>> Could you kindly help to update about this patch?
>>
>
> So is this patch concluded? Are you applying it?
>
> Thanks,
> Aniroop Mathur
>
>> Thank you,
>> Aniroop Mathur
>>
>>
>> On Fri, Mar 11, 2016 at 12:26 AM, Aniroop Mathur
>> <aniroop.mathur@gmail.com> wrote:
>>> Hi Henrik,
>>>
>>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>>>> Hi Dmitry,
>>>>
>>>>>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>>>>> index 8806059..262ef77 100644
>>>>>> --- a/drivers/input/input.c
>>>>>> +++ b/drivers/input/input.c
>>>>>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>>>>>>                 if (dev->num_vals >= 2)
>>>>>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>>>>>>                 dev->num_vals = 0;
>>>>>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>>>>>> -               dev->vals[dev->num_vals++] = input_value_sync;
>>>>>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>>>>>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>>>>>>                 dev->num_vals = 0;
>>>>>>         }
>>>>>
>>>>> This makes sense to me. Henrik?
>>>>
>>>> I went through the commits that made these changes, and I cannot see any strong
>>>> reason to keep it. However, this code path only triggers if no SYN events are
>>>> seen, as in a driver that fails to emit them and consequently fills up the
>>>> buffer. In other words, this change would only affect a device that is already,
>>>> to some degree, broken.
>>>>
>>>> So, the question to Aniroop is: do you see this problem in practise, and in that
>>>> case, for what driver?
>>>>
>>>
>>> Nope. So far I have not dealt with any such driver.
>>> I made this change because it is breaking protocol of SYN_REPORT event code.
>>>
>>> Further from the code, I could deduce that max_vals is just an estimation of
>>> packet_size and it does not guarantee that packet_size is same as max_vals.
>>> So real packet_size can be more than max_vals value and hence we could not
>>> insert SYN_REPORT until packet ends really.
>>> Further, if we consider that there exists a driver or will exist in future
>>> which sets capability of x event code according to which max_value comes out to
>>> y and the real packet size is z i.e. driver wants to send same event codes
>>> again in the same packet, so input event reader would be expecting SYN_REPORT
>>> after z events but due to current code SYN_REPORT will get inserted
>>> automatically after y events, which is a wrong behaviour.
>>>
>>> Thanks,
>>> Aniroop Mathur
>>>
>>>> Henrik
>>>>

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Dmitry Torokhov]

On Fri, Mar 11, 2016 at 12:26:57AM +0530, Aniroop Mathur wrote:
> Hi Henrik,
> 
> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> > Hi Dmitry,
> >
> >>> diff --git a/drivers/input/input.c b/drivers/input/input.c
> >>> index 8806059..262ef77 100644
> >>> --- a/drivers/input/input.c
> >>> +++ b/drivers/input/input.c
> >>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
> >>>                 if (dev->num_vals >= 2)
> >>>                         input_pass_values(dev, dev->vals, dev->num_vals);
> >>>                 dev->num_vals = 0;
> >>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
> >>> -               dev->vals[dev->num_vals++] = input_value_sync;
> >>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
> >>>                 input_pass_values(dev, dev->vals, dev->num_vals);
> >>>                 dev->num_vals = 0;
> >>>         }
> >>
> >> This makes sense to me. Henrik?
> >
> > I went through the commits that made these changes, and I cannot see any strong
> > reason to keep it. However, this code path only triggers if no SYN events are
> > seen, as in a driver that fails to emit them and consequently fills up the
> > buffer. In other words, this change would only affect a device that is already,
> > to some degree, broken.
> >
> > So, the question to Aniroop is: do you see this problem in practise, and in that
> > case, for what driver?
> >
> 
> Nope. So far I have not dealt with any such driver.
> I made this change because it is breaking protocol of SYN_REPORT event code.
> 
> Further from the code, I could deduce that max_vals is just an estimation of
> packet_size and it does not guarantee that packet_size is same as max_vals.
> So real packet_size can be more than max_vals value and hence we could not
> insert SYN_REPORT until packet ends really.
> Further, if we consider that there exists a driver or will exist in future
> which sets capability of x event code according to which max_value comes out to
> y and the real packet size is z i.e. driver wants to send same event codes
> again in the same packet, so input event reader would be expecting SYN_REPORT
> after z events but due to current code SYN_REPORT will get inserted
> automatically after y events, which is a wrong behaviour.

Well, I think I agree with Aniroop that even if driver is to a degree
broken we should not be inserting random SYN_REPORT events into the
stream. I wonder if we should not add WARN_ONCE() there to highlight
potential problems with the way we estimate the number of events.

However I think there is an issue with the patch. If we happen to pass
values just before the final SYN_REPORT sent by the driver then we reset
dev->num_vals to 0 and will essentially suppress the final SYN_REPORT
event, which is not good either.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hello Mr. Torokhov,

First of all, Thank you for your reply.

On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Mar 11, 2016 at 12:26:57AM +0530, Aniroop Mathur wrote:
>> Hi Henrik,
>>
>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>> > Hi Dmitry,
>> >
>> >>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> >>> index 8806059..262ef77 100644
>> >>> --- a/drivers/input/input.c
>> >>> +++ b/drivers/input/input.c
>> >>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>> >>>                 if (dev->num_vals >= 2)
>> >>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>> >>>                 dev->num_vals = 0;
>> >>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>> >>> -               dev->vals[dev->num_vals++] = input_value_sync;
>> >>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>> >>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>> >>>                 dev->num_vals = 0;
>> >>>         }
>> >>
>> >> This makes sense to me. Henrik?
>> >
>> > I went through the commits that made these changes, and I cannot see any strong
>> > reason to keep it. However, this code path only triggers if no SYN events are
>> > seen, as in a driver that fails to emit them and consequently fills up the
>> > buffer. In other words, this change would only affect a device that is already,
>> > to some degree, broken.
>> >
>> > So, the question to Aniroop is: do you see this problem in practise, and in that
>> > case, for what driver?
>> >
>>
>> Nope. So far I have not dealt with any such driver.
>> I made this change because it is breaking protocol of SYN_REPORT event code.
>>
>> Further from the code, I could deduce that max_vals is just an estimation of
>> packet_size and it does not guarantee that packet_size is same as max_vals.
>> So real packet_size can be more than max_vals value and hence we could not
>> insert SYN_REPORT until packet ends really.
>> Further, if we consider that there exists a driver or will exist in future
>> which sets capability of x event code according to which max_value comes out to
>> y and the real packet size is z i.e. driver wants to send same event codes
>> again in the same packet, so input event reader would be expecting SYN_REPORT
>> after z events but due to current code SYN_REPORT will get inserted
>> automatically after y events, which is a wrong behaviour.
>
> Well, I think I agree with Aniroop that even if driver is to a degree
> broken we should not be inserting random SYN_REPORT events into the
> stream. I wonder if we should not add WARN_ONCE() there to highlight
> potential problems with the way we estimate the number of events.
>
> However I think there is an issue with the patch. If we happen to pass
> values just before the final SYN_REPORT sent by the driver then we reset
> dev->num_vals to 0 and will essentially suppress the final SYN_REPORT
> event, which is not good either.
>

Yes, right!

I think it can be fixed by sending the rest of events but not the last event
in case number of events becomes greater than max_vals. The last event will be
saved to be sent in next set of events. This way immediate SYN_REPORT will not
be suppressed and duplicate SYN_REPORT event will not be sent as well.

Change:
@@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
                if (dev->num_vals >= 2)
                        input_pass_values(dev, dev->vals, dev->num_vals);
                dev->num_vals = 0;
-       } else if (dev->num_vals >= dev->max_vals - 2) {
-               dev->vals[dev->num_vals++] = input_value_sync;
-               input_pass_values(dev, dev->vals, dev->num_vals);
-               dev->num_vals = 0;
+       } else if (dev->num_vals == dev->max_vals) {
+                input_pass_values(dev, dev->vals, dev->num_vals - 1);
+                dev->num_vals = 0;
+                dev->vals[dev->num_vals++] = dev->vals[dev->max_vals - 1];
        }

So, does the above patch looks good now?

And may be about WARN_ONCE, do you mean to add something like below in above
code?
WARN_ONCE(1, "Packet did not complete yet but generally expected to be
completed before generation of %d events.\n", dev->max_vals);


Thanks,
Aniroop Mathur

> Thanks.
>
> --
> Dmitry

[PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

As mentioned in documentation, SYN_REPORT should be used to separate two packets
and should not be inserted in between a single packet as otherwise with multiple
SYN_REPORT in a single packet, input reader would not be able to know when the
packet ended really.

Documentation snippet:
* SYN_REPORT:
  - Used to synchronize and separate events into packets of input data changes
    occurring at the same moment in time. For example, motion of a mouse may set
    the REL_X and REL_Y values for one motion, then emit a SYN_REPORT. The next
    motion will emit more REL_X and REL_Y values and send another SYN_REPORT.

Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
 drivers/input/input.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 8806059..799941c 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -401,12 +401,11 @@ static void input_handle_event(struct input_dev *dev,
 		if (dev->num_vals >= 2)
 			input_pass_values(dev, dev->vals, dev->num_vals);
 		dev->num_vals = 0;
-	} else if (dev->num_vals >= dev->max_vals - 2) {
-		dev->vals[dev->num_vals++] = input_value_sync;
-		input_pass_values(dev, dev->vals, dev->num_vals);
+	} else if (dev->num_vals == dev->max_vals) {
+		input_pass_values(dev, dev->vals, dev->num_vals - 1);
 		dev->num_vals = 0;
+		dev->vals[dev->num_vals++] = dev->vals[dev->max_vals - 1];
 	}
-
 }
 
 /**
-- 
2.6.2

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

On Sat, Apr 2, 2016 at 10:31 PM, Aniroop Mathur
<aniroop.mathur@gmail.com> wrote:
> Hello Mr. Torokhov,
>
> First of all, Thank you for your reply.
>
> On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Fri, Mar 11, 2016 at 12:26:57AM +0530, Aniroop Mathur wrote:
>>> Hi Henrik,
>>>
>>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>>> > Hi Dmitry,
>>> >
>>> >>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>>> >>> index 8806059..262ef77 100644
>>> >>> --- a/drivers/input/input.c
>>> >>> +++ b/drivers/input/input.c
>>> >>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>>> >>>                 if (dev->num_vals >= 2)
>>> >>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>>> >>>                 dev->num_vals = 0;
>>> >>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>>> >>> -               dev->vals[dev->num_vals++] = input_value_sync;
>>> >>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>>> >>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>>> >>>                 dev->num_vals = 0;
>>> >>>         }
>>> >>
>>> >> This makes sense to me. Henrik?
>>> >
>>> > I went through the commits that made these changes, and I cannot see any strong
>>> > reason to keep it. However, this code path only triggers if no SYN events are
>>> > seen, as in a driver that fails to emit them and consequently fills up the
>>> > buffer. In other words, this change would only affect a device that is already,
>>> > to some degree, broken.
>>> >
>>> > So, the question to Aniroop is: do you see this problem in practise, and in that
>>> > case, for what driver?
>>> >
>>>
>>> Nope. So far I have not dealt with any such driver.
>>> I made this change because it is breaking protocol of SYN_REPORT event code.
>>>
>>> Further from the code, I could deduce that max_vals is just an estimation of
>>> packet_size and it does not guarantee that packet_size is same as max_vals.
>>> So real packet_size can be more than max_vals value and hence we could not
>>> insert SYN_REPORT until packet ends really.
>>> Further, if we consider that there exists a driver or will exist in future
>>> which sets capability of x event code according to which max_value comes out to
>>> y and the real packet size is z i.e. driver wants to send same event codes
>>> again in the same packet, so input event reader would be expecting SYN_REPORT
>>> after z events but due to current code SYN_REPORT will get inserted
>>> automatically after y events, which is a wrong behaviour.
>>
>> Well, I think I agree with Aniroop that even if driver is to a degree
>> broken we should not be inserting random SYN_REPORT events into the
>> stream. I wonder if we should not add WARN_ONCE() there to highlight
>> potential problems with the way we estimate the number of events.
>>
>> However I think there is an issue with the patch. If we happen to pass
>> values just before the final SYN_REPORT sent by the driver then we reset
>> dev->num_vals to 0 and will essentially suppress the final SYN_REPORT
>> event, which is not good either.
>>
>
> Yes, right!
>
> I think it can be fixed by sending the rest of events but not the last event
> in case number of events becomes greater than max_vals. The last event will be
> saved to be sent in next set of events. This way immediate SYN_REPORT will not
> be suppressed and duplicate SYN_REPORT event will not be sent as well.
>
> Change:
> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>                 if (dev->num_vals >= 2)
>                         input_pass_values(dev, dev->vals, dev->num_vals);
>                 dev->num_vals = 0;
> -       } else if (dev->num_vals >= dev->max_vals - 2) {
> -               dev->vals[dev->num_vals++] = input_value_sync;
> -               input_pass_values(dev, dev->vals, dev->num_vals);
> -               dev->num_vals = 0;
> +       } else if (dev->num_vals == dev->max_vals) {
> +                input_pass_values(dev, dev->vals, dev->num_vals - 1);
> +                dev->num_vals = 0;
> +                dev->vals[dev->num_vals++] = dev->vals[dev->max_vals - 1];
>         }
>
> So, does the above patch looks good now?
>


Hello Mr. Torokhov,

Could you please update about this?
It would be appreciating if you could help out to conclude it quickly.  Thanks!


> And may be about WARN_ONCE, do you mean to add something like below in above
> code?
> WARN_ONCE(1, "Packet did not complete yet but generally expected to be
> completed before generation of %d events.\n", dev->max_vals);
>
>
> Thanks,
> Aniroop Mathur
>
>> Thanks.
>>
>> --
>> Dmitry

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Dmitry Torokhov]

On Wed, Apr 06, 2016 at 08:26:39PM +0530, Aniroop Mathur wrote:
> On Sat, Apr 2, 2016 at 10:31 PM, Aniroop Mathur
> <aniroop.mathur@gmail.com> wrote:
> > Hello Mr. Torokhov,
> >
> > First of all, Thank you for your reply.
> >
> > On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >> On Fri, Mar 11, 2016 at 12:26:57AM +0530, Aniroop Mathur wrote:
> >>> Hi Henrik,
> >>>
> >>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> >>> > Hi Dmitry,
> >>> >
> >>> >>> diff --git a/drivers/input/input.c b/drivers/input/input.c
> >>> >>> index 8806059..262ef77 100644
> >>> >>> --- a/drivers/input/input.c
> >>> >>> +++ b/drivers/input/input.c
> >>> >>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
> >>> >>>                 if (dev->num_vals >= 2)
> >>> >>>                         input_pass_values(dev, dev->vals, dev->num_vals);
> >>> >>>                 dev->num_vals = 0;
> >>> >>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
> >>> >>> -               dev->vals[dev->num_vals++] = input_value_sync;
> >>> >>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
> >>> >>>                 input_pass_values(dev, dev->vals, dev->num_vals);
> >>> >>>                 dev->num_vals = 0;
> >>> >>>         }
> >>> >>
> >>> >> This makes sense to me. Henrik?
> >>> >
> >>> > I went through the commits that made these changes, and I cannot see any strong
> >>> > reason to keep it. However, this code path only triggers if no SYN events are
> >>> > seen, as in a driver that fails to emit them and consequently fills up the
> >>> > buffer. In other words, this change would only affect a device that is already,
> >>> > to some degree, broken.
> >>> >
> >>> > So, the question to Aniroop is: do you see this problem in practise, and in that
> >>> > case, for what driver?
> >>> >
> >>>
> >>> Nope. So far I have not dealt with any such driver.
> >>> I made this change because it is breaking protocol of SYN_REPORT event code.
> >>>
> >>> Further from the code, I could deduce that max_vals is just an estimation of
> >>> packet_size and it does not guarantee that packet_size is same as max_vals.
> >>> So real packet_size can be more than max_vals value and hence we could not
> >>> insert SYN_REPORT until packet ends really.
> >>> Further, if we consider that there exists a driver or will exist in future
> >>> which sets capability of x event code according to which max_value comes out to
> >>> y and the real packet size is z i.e. driver wants to send same event codes
> >>> again in the same packet, so input event reader would be expecting SYN_REPORT
> >>> after z events but due to current code SYN_REPORT will get inserted
> >>> automatically after y events, which is a wrong behaviour.
> >>
> >> Well, I think I agree with Aniroop that even if driver is to a degree
> >> broken we should not be inserting random SYN_REPORT events into the
> >> stream. I wonder if we should not add WARN_ONCE() there to highlight
> >> potential problems with the way we estimate the number of events.
> >>
> >> However I think there is an issue with the patch. If we happen to pass
> >> values just before the final SYN_REPORT sent by the driver then we reset
> >> dev->num_vals to 0 and will essentially suppress the final SYN_REPORT
> >> event, which is not good either.
> >>
> >
> > Yes, right!
> >
> > I think it can be fixed by sending the rest of events but not the last event
> > in case number of events becomes greater than max_vals. The last event will be
> > saved to be sent in next set of events. This way immediate SYN_REPORT will not
> > be suppressed and duplicate SYN_REPORT event will not be sent as well.
> >
> > Change:
> > @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
> >                 if (dev->num_vals >= 2)
> >                         input_pass_values(dev, dev->vals, dev->num_vals);
> >                 dev->num_vals = 0;
> > -       } else if (dev->num_vals >= dev->max_vals - 2) {
> > -               dev->vals[dev->num_vals++] = input_value_sync;
> > -               input_pass_values(dev, dev->vals, dev->num_vals);
> > -               dev->num_vals = 0;
> > +       } else if (dev->num_vals == dev->max_vals) {
> > +                input_pass_values(dev, dev->vals, dev->num_vals - 1);
> > +                dev->num_vals = 0;
> > +                dev->vals[dev->num_vals++] = dev->vals[dev->max_vals - 1];
> >         }
> >
> > So, does the above patch looks good now?
> >

No, consider what will happen if you need to switch slot when your queue
is at dev->max_vals - 1. With your patch you will end up with out of
bounds write.

> 
> 
> Hello Mr. Torokhov,
> 
> Could you please update about this?
> It would be appreciating if you could help out to conclude it quickly.  Thanks!

I am not sure what the urgency is. It is more of a theoretical problem
ans so far the proposed solutions were actually introducing more
problems than they were solving.

I am sorry, bit this particular topic is not a priority for me.

> 
> 
> > And may be about WARN_ONCE, do you mean to add something like below in above
> > code?
> > WARN_ONCE(1, "Packet did not complete yet but generally expected to be
> > completed before generation of %d events.\n", dev->max_vals);
> >
> >
> > Thanks,
> > Aniroop Mathur
> >
> >> Thanks.
> >>
> >> --
> >> Dmitry

Thanks.

-- 
Dmitry

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

On Wed, Apr 6, 2016 at 11:08 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Wed, Apr 06, 2016 at 08:26:39PM +0530, Aniroop Mathur wrote:
>> On Sat, Apr 2, 2016 at 10:31 PM, Aniroop Mathur
>> <aniroop.mathur@gmail.com> wrote:
>> > Hello Mr. Torokhov,
>> >
>> > First of all, Thank you for your reply.
>> >
>> > On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> >> On Fri, Mar 11, 2016 at 12:26:57AM +0530, Aniroop Mathur wrote:
>> >>> Hi Henrik,
>> >>>
>> >>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>> >>> > Hi Dmitry,
>> >>> >
>> >>> >>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> >>> >>> index 8806059..262ef77 100644
>> >>> >>> --- a/drivers/input/input.c
>> >>> >>> +++ b/drivers/input/input.c
>> >>> >>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>> >>> >>>                 if (dev->num_vals >= 2)
>> >>> >>>                         input_pass_values(dev, dev->vals, dev->num_vals);
>> >>> >>>                 dev->num_vals = 0;
>> >>> >>> -       } else if (dev->num_vals >= dev->max_vals - 2) {
>> >>> >>> -               dev->vals[dev->num_vals++] = input_value_sync;
>> >>> >>> +       } else if (dev->num_vals >= dev->max_vals - 1) {
>> >>> >>>                 input_pass_values(dev, dev->vals, dev->num_vals);
>> >>> >>>                 dev->num_vals = 0;
>> >>> >>>         }
>> >>> >>
>> >>> >> This makes sense to me. Henrik?
>> >>> >
>> >>> > I went through the commits that made these changes, and I cannot see any strong
>> >>> > reason to keep it. However, this code path only triggers if no SYN events are
>> >>> > seen, as in a driver that fails to emit them and consequently fills up the
>> >>> > buffer. In other words, this change would only affect a device that is already,
>> >>> > to some degree, broken.
>> >>> >
>> >>> > So, the question to Aniroop is: do you see this problem in practise, and in that
>> >>> > case, for what driver?
>> >>> >
>> >>>
>> >>> Nope. So far I have not dealt with any such driver.
>> >>> I made this change because it is breaking protocol of SYN_REPORT event code.
>> >>>
>> >>> Further from the code, I could deduce that max_vals is just an estimation of
>> >>> packet_size and it does not guarantee that packet_size is same as max_vals.
>> >>> So real packet_size can be more than max_vals value and hence we could not
>> >>> insert SYN_REPORT until packet ends really.
>> >>> Further, if we consider that there exists a driver or will exist in future
>> >>> which sets capability of x event code according to which max_value comes out to
>> >>> y and the real packet size is z i.e. driver wants to send same event codes
>> >>> again in the same packet, so input event reader would be expecting SYN_REPORT
>> >>> after z events but due to current code SYN_REPORT will get inserted
>> >>> automatically after y events, which is a wrong behaviour.
>> >>
>> >> Well, I think I agree with Aniroop that even if driver is to a degree
>> >> broken we should not be inserting random SYN_REPORT events into the
>> >> stream. I wonder if we should not add WARN_ONCE() there to highlight
>> >> potential problems with the way we estimate the number of events.
>> >>
>> >> However I think there is an issue with the patch. If we happen to pass
>> >> values just before the final SYN_REPORT sent by the driver then we reset
>> >> dev->num_vals to 0 and will essentially suppress the final SYN_REPORT
>> >> event, which is not good either.
>> >>
>> >
>> > Yes, right!
>> >
>> > I think it can be fixed by sending the rest of events but not the last event
>> > in case number of events becomes greater than max_vals. The last event will be
>> > saved to be sent in next set of events. This way immediate SYN_REPORT will not
>> > be suppressed and duplicate SYN_REPORT event will not be sent as well.
>> >
>> > Change:
>> > @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>> >                 if (dev->num_vals >= 2)
>> >                         input_pass_values(dev, dev->vals, dev->num_vals);
>> >                 dev->num_vals = 0;
>> > -       } else if (dev->num_vals >= dev->max_vals - 2) {
>> > -               dev->vals[dev->num_vals++] = input_value_sync;
>> > -               input_pass_values(dev, dev->vals, dev->num_vals);
>> > -               dev->num_vals = 0;
>> > +       } else if (dev->num_vals == dev->max_vals) {
>> > +                input_pass_values(dev, dev->vals, dev->num_vals - 1);
>> > +                dev->num_vals = 0;
>> > +                dev->vals[dev->num_vals++] = dev->vals[dev->max_vals - 1];
>> >         }
>> >
>> > So, does the above patch looks good now?
>> >
>
> No, consider what will happen if you need to switch slot when your queue
> is at dev->max_vals - 1. With your patch you will end up with out of
> bounds write.
>

Sorry, I know very less about MT protocol, so could not catch this.
I have worked only on normal input device drivers.
input_abs_set_val(dev, ABS_MT_SLOT, mt->slot); --> I guess I missed this? :(

I have modified condition to handle it, so does it look fine now?

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 8806059..799941c 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -401,12 +401,11 @@ static void input_handle_event(struct input_dev *dev,
                if (dev->num_vals >= 2)
                        input_pass_values(dev, dev->vals, dev->num_vals);
                dev->num_vals = 0;
-       } else if (dev->num_vals >= dev->max_vals - 2) {
-               dev->vals[dev->num_vals++] = input_value_sync;
-               input_pass_values(dev, dev->vals, dev->num_vals);
+       } else if (dev->num_vals >= dev->max_vals - 1) {
+               input_pass_values(dev, dev->vals, dev->num_vals - 1);
                dev->num_vals = 0;
+               dev->vals[dev->num_vals++] = dev->vals[dev->num_vals - 1];
        }
-
 }

>>
>>
>> Hello Mr. Torokhov,
>>
>> Could you please update about this?
>> It would be appreciating if you could help out to conclude it quickly.  Thanks!
>
> I am not sure what the urgency is. It is more of a theoretical problem
> ans so far the proposed solutions were actually introducing more
> problems than they were solving.
>
> I am sorry, bit this particular topic is not a priority for me.
>

There is no hurry at all. :-) As you know request is made a long time ago,
so I am only very curious to complete it.

>>
>>
>> > And may be about WARN_ONCE, do you mean to add something like below in above
>> > code?
>> > WARN_ONCE(1, "Packet did not complete yet but generally expected to be
>> > completed before generation of %d events.\n", dev->max_vals);
>> >
>> >
>> > Thanks,
>> > Aniroop Mathur
>> >
>> >> Thanks.
>> >>
>> >> --
>> >> Dmitry
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Henrik Rydberg]

Hi Aniroop,

>> I am not sure what the urgency is. It is more of a theoretical problem
>> ans so far the proposed solutions were actually introducing more
>> problems than they were solving.
>>
>> I am sorry, bit this particular topic is not a priority for me.
>>
> 
> There is no hurry at all. :-) As you know request is made a long time ago,
> so I am only very curious to complete it.

This kind of patch is not liked by any maintainer, because it does not solve any
immediate problem, but instead may create one. If such a simple patch takes
three of four tries to look right, it only adds to the perception that the code
is best left alone.

I think the solution at this stage is to say no to this patch.

If there is ever a driver for which the input_estimate_events_per_packet()
function returns less than the actual maximum number of events per frame, this
issue can be revisited and resolved in a number of different ways.

Sorry, and thanks for your work.

Henrik

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

Hello Mr. Henrik,

On Thu, Apr 7, 2016 at 1:21 AM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> Hi Aniroop,
>
>>> I am not sure what the urgency is. It is more of a theoretical problem
>>> ans so far the proposed solutions were actually introducing more
>>> problems than they were solving.
>>>
>>> I am sorry, bit this particular topic is not a priority for me.
>>>
>>
>> There is no hurry at all. :-) As you know request is made a long time ago,
>> so I am only very curious to complete it.
>
> This kind of patch is not liked by any maintainer, because it does not solve any
> immediate problem, but instead may create one. If such a simple patch takes
> three of four tries to look right, it only adds to the perception that the code
> is best left alone.
>
> I think the solution at this stage is to say no to this patch.
>
> If there is ever a driver for which the input_estimate_events_per_packet()
> function returns less than the actual maximum number of events per frame, this
> issue can be revisited and resolved in a number of different ways.
>
> Sorry, and thanks for your work.
>

Well, I agree this code might not be used by any driver so far.
But if some driver developer writes such a driver, then it definitely cannot
work well because of the bug in input subsystem code. So I am afraid that it
is not a good idea to wait for someone to report this bug when we already know
that the bug does exist in input core.

Secondly, I submitted this patch not only because it breaks protocol of
SYN_REPORT event but also because without this bug fix, another bug could not
be concluded which depends on when the input event packet ended really.
Bug:
Input: evdev: fix bug of dropping valid packet after syn_dropped event
https://patchwork.kernel.org/patch/8083641/
So to fix this bug, we need to fix SYN_REPORT bug first.

It would be appreciating of you if you could give it one more spin.


> Henrik
>

Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data

[Aniroop Mathur]

On Thu, Apr 7, 2016 at 2:13 AM, Aniroop Mathur <aniroop.mathur@gmail.com> wrote:
> Hello Mr. Henrik,
>
> On Thu, Apr 7, 2016 at 1:21 AM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>> Hi Aniroop,
>>
>>>> I am not sure what the urgency is. It is more of a theoretical problem
>>>> ans so far the proposed solutions were actually introducing more
>>>> problems than they were solving.
>>>>
>>>> I am sorry, bit this particular topic is not a priority for me.
>>>>
>>>
>>> There is no hurry at all. :-) As you know request is made a long time ago,
>>> so I am only very curious to complete it.
>>
>> This kind of patch is not liked by any maintainer, because it does not solve any
>> immediate problem, but instead may create one. If such a simple patch takes
>> three of four tries to look right, it only adds to the perception that the code
>> is best left alone.
>>
>> I think the solution at this stage is to say no to this patch.
>>
>> If there is ever a driver for which the input_estimate_events_per_packet()
>> function returns less than the actual maximum number of events per frame, this
>> issue can be revisited and resolved in a number of different ways.
>>
>> Sorry, and thanks for your work.
>>
>
> Well, I agree this code might not be used by any driver so far.
> But if some driver developer writes such a driver, then it definitely cannot
> work well because of the bug in input subsystem code. So I am afraid that it
> is not a good idea to wait for someone to report this bug when we already know
> that the bug does exist in input core.
>
> Secondly, I submitted this patch not only because it breaks protocol of
> SYN_REPORT event but also because without this bug fix, another bug could not
> be concluded which depends on when the input event packet ended really.
> Bug:
> Input: evdev: fix bug of dropping valid packet after syn_dropped event
> https://patchwork.kernel.org/patch/8083641/
> So to fix this bug, we need to fix SYN_REPORT bug first.
>
> It would be appreciating of you if you could give it one more spin.
>
>

Hello Mr. Torokhov,

Would you please update further about this patch?

Thanks,
Aniroop Mathur