diff mbox

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

Message ID CADYu308OF98jdbsqpSVdQpTa0sEafuQmVuC=ttN3-1u59_QvmA@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aniroop Mathur April 6, 2016, 7:09 p.m. UTC 
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?

>>
>> 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
--
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

Comments

Henrik Rydberg April 6, 2016, 7:51 p.m. UTC | #1 
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

--
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
Aniroop Mathur April 6, 2016, 8:43 p.m. UTC | #2 
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
>
--
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
Aniroop Mathur April 27, 2016, 2:59 p.m. UTC | #3 
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
--
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/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];
        }
-
 }

>>