| Message ID | 1457372672-884-1-git-send-email-a.mathur@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- 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
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?
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 -- 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
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 > -- 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
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 >> -- 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
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 >>> -- 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
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 >>>> -- 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
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.
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 -- 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
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 -- 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
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.
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; }
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(-)