Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370002 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-04-02 19:10 +0200
    [PATCH] Input: Do not add SYN_REPORT in between a single packet data Aniroop Mathur <a.mathur@samsung.com> - 2016-04-04 19:30 +0200
    Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-04-06 17:00 +0200
      Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet  data Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2016-04-06 19:40 +0200
        Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-04-06 21:10 +0200
          Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet  data Henrik Rydberg <rydberg@bitmath.org> - 2016-04-06 22:00 +0200
            Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-04-06 22:50 +0200

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

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

[toc] | [next] | [standalone]

#1370800

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

[toc] | [prev] | [next] | [standalone]

#1372579

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1372743

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1372807

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
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web