Re: Re: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
On Fri, Feb 24, 2017 at 5:06 PM, Aniroop Mathur wrote:
>> Hi
>>
>> On Tue, Jan 31, 2017 at 5:09 PM, Aniroop Mathur wrote:
>>> On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann
>>> wrote:
>>>> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur
>>>> wrote:
>>>>> continue;
>>>>> } else if (head != i) {
>>>>> /* move entry to fill the gap */
>>>>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
>>>>> *client, unsigned int type)
>>>>> }
>>>>>
>>>>> client->head = head;
>>>>> + return drop_count;
>>>>> }
>>>>>
>>>>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>>>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client
>>>>> *client,
>>>>> int ret;
>>>>> unsigned long *mem;
>>>>> size_t len;
>>>>> + unsigned int drop_count = 0;
>>>>>
>>>>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>>>> mem = kmalloc(len, GFP_KERNEL);
>>>>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
>>>>> *client,
>>>>>
>>>>> spin_unlock(&dev->event_lock);
>>>>>
>>>>> - __evdev_flush_queue(client, type);
>>>>> + drop_count = __evdev_flush_queue(client, type);
>>>>>
>>>>> spin_unlock_irq(&client->buffer_lock);
>>>>>
>>>>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>>>>> - if (ret < 0)
>>>>> + if (ret < 0 && drop_count > 0)
>>>>> evdev_queue_syn_dropped(client);
>>>>
>>>> I don't see the point. If bits_to_user() fails, you get EFAULT.
>>>> User-space cannot assume anything is still valid if they get EFAULT.
>>>> This is not like ENOMEM or other errors that you can recover from.
>>>> EFAULT means _programming_ error, not runtime exception.
>>>>
>>>> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
>>>> whenever a syscall returns an error, you can rely on it to not have
>>>> modified state. EFAULT is an exception on most paths, since it might
>>>> occur when copying over results, and it is overly expensive to handle
>>>> EFAULT gracefully there (you'd have to copy _results_ to user-space,
>>>> before making them visible to the system).
>>>>
>>>> Long story short: I don't see the point in this patch. This path is
>>>> *never* triggered, except if your client is buggy. And even if you
>>>> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>>>>
>>>> Care to elaborate why exactly you want this modification?
>>>> David
>>>>
>>>
>>> Sure, I will elaborate for you.
>>> Basically, the bug is that if the last event dropped in the queue is
>>> EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
>>> userspace client will ignore the next complete event packet as per rule
>>> defined in the document although the client should not ignore the events
>>> until EV_SYN/SYN_REPORT because the events for this case are not partial
>>> events but the full packet indeed. So we need to make sure whenever this
>>> happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
>>> event so that client do not ignore the next full packet.
>>> I already fixed this bug and you can see the patch here (not submitted yet)
>>> https://patchwork.kernel.org/patch/9362233/
>>>
>>> For this patch, we had no problem with the case of kernel buffer overrun and
>>> also had no problem for the case of clock change request, but only had
>>> problem
>>> for the case of EVIOCG ioctl call which I have already explained in this
>>> patch
>>> description.
>>> In short, if we insert SYN_DROPPED event wrongly then client will ignore
>>> events until SYN_REPORT event which we do not want to happen. So that is
>>> why I want this modification in order to have correct insertion of
>>> SYN_DROPPED event and hence go ahead with another patch I mentioned above.
>
RE: Re: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
> Hi
>
> On Tue, Jan 31, 2017 at 5:09 PM, Aniroop Mathur wrote:
>> On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann
>> wrote:
>>> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur
>>> wrote:
>>>> continue;
>>>> } else if (head != i) {
>>>> /* move entry to fill the gap */
>>>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
>>>> *client, unsigned int type)
>>>> }
>>>>
>>>> client->head = head;
>>>> + return drop_count;
>>>> }
>>>>
>>>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client
>>>> *client,
>>>> int ret;
>>>> unsigned long *mem;
>>>> size_t len;
>>>> + unsigned int drop_count = 0;
>>>>
>>>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>>>> mem = kmalloc(len, GFP_KERNEL);
>>>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
>>>> *client,
>>>>
>>>> spin_unlock(&dev->event_lock);
>>>>
>>>> - __evdev_flush_queue(client, type);
>>>> + drop_count = __evdev_flush_queue(client, type);
>>>>
>>>> spin_unlock_irq(&client->buffer_lock);
>>>>
>>>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>>>> - if (ret < 0)
>>>> + if (ret < 0 && drop_count > 0)
>>>> evdev_queue_syn_dropped(client);
>>>
>>> I don't see the point. If bits_to_user() fails, you get EFAULT.
>>> User-space cannot assume anything is still valid if they get EFAULT.
>>> This is not like ENOMEM or other errors that you can recover from.
>>> EFAULT means _programming_ error, not runtime exception.
>>>
>>> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
>>> whenever a syscall returns an error, you can rely on it to not have
>>> modified state. EFAULT is an exception on most paths, since it might
>>> occur when copying over results, and it is overly expensive to handle
>>> EFAULT gracefully there (you'd have to copy _results_ to user-space,
>>> before making them visible to the system).
>>>
>>> Long story short: I don't see the point in this patch. This path is
>>> *never* triggered, except if your client is buggy. And even if you
>>> trigger it, placing SYN_DROPPED in the queue does no harm at all.
>>>
>>> Care to elaborate why exactly you want this modification?
>>> David
>>>
>>
>> Sure, I will elaborate for you.
>> Basically, the bug is that if the last event dropped in the queue is
>> EV_SYN/SYN_REPORT and next event inserted is EV_SYN/SYN_DROPPED, then the
>> userspace client will ignore the next complete event packet as per rule
>> defined in the document although the client should not ignore the events
>> until EV_SYN/SYN_REPORT because the events for this case are not partial
>> events but the full packet indeed. So we need to make sure whenever this
>> happens we immediately insert another EV_SYN/SYN_REPORT after EV_SYN/DROPPED
>> event so that client do not ignore the next full packet.
>> I already fixed this bug and you can see the patch here (not submitted yet)
>> https://patchwork.kernel.org/patch/9362233/
>>
>> For this patch, we had no problem with the case of kernel buffer overrun and
>> also had no problem for the case of clock change request, but only had
>> problem
>> for the case of EVIOCG ioctl call which I have already explained in this
>> patch
>> description.
>> In short, if we insert SYN_DROPPED event wrongly then client will ignore
>> events until SYN_REPORT event which we do not want to happen. So that is
>> why I want this modification in order to have correct insertion of
>> SYN_DROPPED event and hence go ahead with another patch I mentioned above.
>
> Fair enough. A SYN_DROPPED should be followed by a SYN_REPORT. The
> normal insertion path guarantees that (since it keeps the last event
> alive), the other 2 fake SYN_DROPPED insertions don't. But...
>
the other 2?
Anyways, only problematic case for SYN_DROPPED event was EVIOCG[*] ioctl,
for which you mentioned below that it is completely fine to not add
SYN_DROPPE
RE: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann wrote:
> Hi
>
> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur wrote:
>> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
>> then SYN_DROPPED event is inserted in the event queue always.
>>
>> However, it is not compulsory that some events are flushed out on every
>> EVIOCG[type] ioctl call like in case of empty event queue and in case when
>> EVIOCG[type] ioctl is issued for say A type of events but event queue does
>> not have any A type of events but some other type of events.
>>
>> Therefore, insert SYN_DROPPED event only when some events have been flushed
>> out from event queue plus bits_to_user fails.
>>
>> Signed-off-by: Aniroop Mathur
>> ---
>> drivers/input/evdev.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..f8b295e 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client
>> *client,
>> }
>>
>> /* flush queued events of type @type, caller must hold client->buffer_lock
>> */
>> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int
>> type)
>> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
>> + unsigned int type)
>> {
>> unsigned int i, head, num;
>> + unsigned int drop_count = 0;
>> unsigned int mask = client->bufsize - 1;
>> bool is_report;
>> struct input_event *ev;
>> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>>
>> if (ev->type == type) {
>> /* drop matched entry */
>> + drop_count++;
>> continue;
>> } else if (is_report && !num) {
>> /* drop empty SYN_REPORT groups */
>> + drop_count++;
>
> Dropping an empty report-group should not be considered in
> `drop_count` here. Empty report groups carry no information and should
> not affect client's state. You should only increment `drop_count` in
> the first block.
>
All right, I am good with increasing the drop_count for only matched entries.
>> continue;
>> } else if (head != i) {
>> /* move entry to fill the gap */
>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>> }
>>
>> client->head = head;
>> + return drop_count;
>> }
>>
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client
>> *client,
>> int ret;
>> unsigned long *mem;
>> size_t len;
>> + unsigned int drop_count = 0;
>>
>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>> mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
>> *client,
>>
>> spin_unlock(&dev->event_lock);
>>
>> - __evdev_flush_queue(client, type);
>> + drop_count = __evdev_flush_queue(client, type);
>>
>> spin_unlock_irq(&client->buffer_lock);
>>
>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> - if (ret < 0)
>> + if (ret < 0 && drop_count > 0)
>> evdev_queue_syn_dropped(client);
>
> I don't see the point. If bits_to_user() fails, you get EFAULT.
> User-space cannot assume anything is still valid if they get EFAULT.
> This is not like ENOMEM or other errors that you can recover from.
> EFAULT means _programming_ error, not runtime exception.
>
> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
> whenever a syscall returns an error, you can rely on it to not have
> modified state. EFAULT is an exception on most paths, since it might
> occur when copying over results, and it is overly expensive to handle
> EFAULT gracefully there (you'd have to copy _results_ to user-space,
> before making them visible to the system).
>
> Long story short: I don't see the point in this patch. This path is
> *never* triggered, except if your client is buggy. And even if you
> trigger it, placi
Re: Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event
On Tue, Nov 22, 2016 at 12:17 AM, Aniroop Mathur wrote:
> Hello Mr. Torokhov,
>
>
>>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>> {
>>> struct input_event ev;
>>> +struct input_event *last_ev;
>>> ktime_t time;
>>> +unsigned int mask = client->bufsize - 1;
>>> +
>>> +/* capture last event stored in the buffer */
>>> +last_ev = &client->buffer[(client->head - 1) & mask];
>
>> I am uneasy with this "looking back" into the queue. How can we be sure
>> that we are looking at the right event? As far as I can tell we do not
>> know if that event has been consumed or not.
>
>
Hello Mr. Dmitry Torokhov,
Along with the below point I already mentioned,
I thought more about this doubt - whether the last event is consumed or not.
Actually, it does not matter whether last event is consumed or not.
In both cases, we need to insert the SYN_REPORT event right after SYN_DROPPED
event. So even if last event which if is SYN_REPORT is consumed by the client
already and next event inserted is SYN_DROPPED, then also we need to insert
SYN_REPORT event.
So it does not matter whether last event is consumed or not consumed.
The only thing matter is that whether SYN_DROPPED event is inserted
correctly or not.
>
> Well, I think that for an exceptional case where even if last event is
> consumed then also it is the right event to proceed with.
>
> Before mentioning then exceptional case, there is a need to fix a bug in
> calling evdev_queue_syn_dropped in evdev_handle_get_val function.
> The bug is:
> Currently we are calling evdev_queue_syn_dropped without taking care
> whether some events have actually been flushed out or not by calling
> __evdev_flush_queue function. But ideally we should call
> evdev_queue_syn_dropped only when some events are being flushed out.
> For example: If client requests for EV_KEY events and queue only has
> EV_LED type of events, then it is possible that bits_to_user fails
> but no events get flushed out from queue and hence we should not be
> inserting SYN_DROPPED event for this case.
> So to fix this,
> I think we need to return the number of events dropped from function
> __evdev_flush_queue, say n is that value. So only if n > 0, then only
> evdev_queue_syn_dropped should be called.
> - __evdev_flush_queue(client, type);
> + n = __evdev_flush_queue(client, type);
> - if (ret < 0)
> + if (ret < 0 && n > 0)
> evdev_queue_syn_dropped(client);
>
> Along with this bug fix, it will also handle the case when queue is
> empty at time of invoking EVIOCG[type] ioctl call so after including
> this fix, we can remove explicit handling of queue empty case from this patch.
>
> After having this change, that exceptional case where even if the last event
> is consumed, it is still the right event is:
> Suppose client request from EV_KEY type of events using EVIOCG[type] ioctl
> call
> and queue does contain EV_KEY type of events and bits_to_user fails too.
> Now, after this when we invoke evdev_queue_syn_dropped function, there is a
> chance that queue get fully consumed i.e. head == tail. So last event is
> consumed as well. But since we need to send SYN_DROPPED event for this case
> to indicate to user space that some events have been dropped, so we also have
> to check whether we need to insert a SYN_REPORT event or not right after
> SYN_DROPPED event using that last consumed event. And I think that it is for
> sure that this last event is actually SYN_REPORT event since input core
> always send events in packets so SYN_REPORT is always the last event forwarded
> by input core to evdev.
>
> Does the above mentioned points seem okay to you?
>
>
> --
> Best Regards,
> Aniroop Mathur
>
>
> - Original Message -
> Sender : Dmitry Torokhov
> Date : 2016-11-20 00:30 (GMT+5:30)
> Title : Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after
> syn_dropped event
>
> Hi Anoroop,
>
> On Wed, Oct 05, 2016 at 12:42:56AM +0530, Aniroop Mathur wrote:
>> If last event dropped in the old queue was EV_SYN/SYN_REPORT, then lets
>> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>> so that clients would not ignore next valid full packet events.
>>
>> Signed-off-by: Aniroop Mathur
>>
>> Difference from v8:
>> Added check for handling EVIOCG[type] ioctl for queue empty case
>> ---
>> drivers/input/evdev.c | 52
>> ++-
>> 1 file changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/dr
RE: Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
On Sun, Jan 22, 2017 at 6:45 PM, David Herrmann wrote:
> Hi
>
> On Thu, Nov 24, 2016 at 9:11 PM, Aniroop Mathur wrote:
>> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
>> then SYN_DROPPED event is inserted in the event queue always.
>>
>> However, it is not compulsory that some events are flushed out on every
>> EVIOCG[type] ioctl call like in case of empty event queue and in case when
>> EVIOCG[type] ioctl is issued for say A type of events but event queue does
>> not have any A type of events but some other type of events.
>>
>> Therefore, insert SYN_DROPPED event only when some events have been flushed
>> out from event queue plus bits_to_user fails.
>>
>> Signed-off-by: Aniroop Mathur
>> ---
>> drivers/input/evdev.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..f8b295e 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client
>> *client,
>> }
>>
>> /* flush queued events of type @type, caller must hold client->buffer_lock
>> */
>> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int
>> type)
>> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
>> + unsigned int type)
>> {
>> unsigned int i, head, num;
>> + unsigned int drop_count = 0;
>> unsigned int mask = client->bufsize - 1;
>> bool is_report;
>> struct input_event *ev;
>> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>>
>> if (ev->type == type) {
>> /* drop matched entry */
>> + drop_count++;
>> continue;
>> } else if (is_report && !num) {
>> /* drop empty SYN_REPORT groups */
>> + drop_count++;
>
> Dropping an empty report-group should not be considered in
> `drop_count` here. Empty report groups carry no information and should
> not affect client's state. You should only increment `drop_count` in
> the first block.
>
All right, I am good with increasing the drop_count for only matched entries.
>> continue;
>> } else if (head != i) {
>> /* move entry to fill the gap */
>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>> }
>>
>> client->head = head;
>> + return drop_count;
>> }
>>
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client
>> *client,
>> int ret;
>> unsigned long *mem;
>> size_t len;
>> + unsigned int drop_count = 0;
>>
>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>> mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
>> *client,
>>
>> spin_unlock(&dev->event_lock);
>>
>> - __evdev_flush_queue(client, type);
>> + drop_count = __evdev_flush_queue(client, type);
>>
>> spin_unlock_irq(&client->buffer_lock);
>>
>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> - if (ret < 0)
>> + if (ret < 0 && drop_count > 0)
>> evdev_queue_syn_dropped(client);
>
> I don't see the point. If bits_to_user() fails, you get EFAULT.
> User-space cannot assume anything is still valid if they get EFAULT.
> This is not like ENOMEM or other errors that you can recover from.
> EFAULT means _programming_ error, not runtime exception.
>
> IOW, EFAULT is special nearly everywhere in the kernel. Usually,
> whenever a syscall returns an error, you can rely on it to not have
> modified state. EFAULT is an exception on most paths, since it might
> occur when copying over results, and it is overly expensive to handle
> EFAULT gracefully there (you'd have to copy _results_ to user-space,
> before making them visible to the system).
>
> Long story short: I don't see the point in this patch. This path is
> *never* triggered, except if your client is buggy. And even if you
> trigger it, placi
Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs
Hello Mr. Vojtech Pavlik, So then is the the behavior being checked on large sleeps as you mentioned? Along with this, like you said that replacing mdelay with usleep_range would be even more interesting so if you would like a patch for that as well to check the behavior then that could be sent to you as well. Thank you. -- Aniroop Mathur On Sat, Dec 3, 2016 at 11:20 PM, Aniroop Mathur wrote: > On Tue, Nov 29, 2016 at 12:29 PM, vojt...@ucw.cz wrote: >> On Mon, Nov 28, 2016 at 01:49:31PM +, Aniroop Mathur wrote: >>> Hello Mr. Vojtech Pavlik, >>> >>> On 28 Nov 2016 17:23, "vojt...@ucw.cz" wrote: >>> > >>> > Hi. >>> > >>> > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer >>> > sleep doesn't matter. In the initilization sequence - first chunk of >>> > your patch - a way too long delay could in theory make the device fail >>> > to initialize. What's critical is that the mdelay() calls are precise. >>> > >>> > One day I'll open my box of old joystick and re-test these drivers to >>> > see if they survived the years of kernel infrastructure updates ... >>> > >>> > Vojtech >>> > >>> >>> Well, it seems to me that there is some kind of confusion, so I'd like to >>> clarify things about this patch. >>> As you have mentioned that in the initialization sequence, long delay could >>> in theory make the device fail to initialize - This patch actually solves >>> this problem. >>> msleep is built on jiffies / legacy timers and usleep_range is built on top >>> of hrtimers so the wakeup will be precise. >>> Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt >>> >>> For example in initialization sequence, if we use msleep(4), then the >>> process >>> could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on >>> machine architecture. Like on a machine with tick rate / HZ is defined to >>> be >>> 100 so msleep(4) will make the process to sleep for minimum 10 ms. >>> Whereas usleep_range(4000, 4100) will make sure that the process do not >>> sleep >>> for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is >>> not. >>> >>> Originally, I added you in this patch to request you if you could help to >>> test this patch or provide contact points of individuals who could help >>> to test this patch as we do not have the hardware available with us? >>> Like this driver, we also need to test other joystick drivers as well like >>> gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have >>> similar problem. >>> Original patch link - https://patchwork.kernel.org/patch/9446201/ >>> As we do not have hardware available, so we decided to split patch into >>> individual drivers and request to person who could support to test this >>> patch >>> >>> I have also applied the same patch in our driver whose hardware is >>> available >>> with me and I found that wake up time became precise indeed and so I >>> decided to apply the same fix in other input subsystem drivers as well. >> >> I do understand what you're trying to achieve. Both ADI_DATA_DELAY and >> ADI_INIT_DELAY are specified as minimum delays. Waiting longer doesn't >> cause any trouble, so the patch doesn't need to change that. >> >> In the initialization sequence, it probably doesn't matter either >> whether we wait longer, hence the distinction between msleep() and >> mdelay() based on positive/negative numbers. The mdelay() needs to be >> exact and the msleep() can be longer. How much longer before it disturbs >> the init sequence I'm not sure, probably quite a bit. >> >> The driver was written a long time before hrtimers existed and as such >> it was written expecting that msleep() can take a longer time. >> > > Well I agree that waiting longer shall not cause any trouble. However, using > usleep_range does not cause any harm here either. In fact, usleep_range is > more advantageous to use here as it makes the wake up more precise than > before. So we have little improved connection / initialization time than > before which is a good thing indeed as it improves response time. > Also, same is being used by new device drivers and now some old device > drivers have also changed to ulseep_range for 1- 10 ms range. > Also, it is recommended and mentioned in the
Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
Dear Mr. Dmitry Torokhov,
Could you please update about this patch?
Thank you.
-- Aniroop Mathur
On Wed, Dec 14, 2016 at 11:54 PM, Aniroop Mathur
wrote:
> Hello Mr. Torokhov,
>
> Would you kindly update about this patch?
> Thanks!
>
> Best Regards,
> Aniroop Mathur
>
>
> On Fri, Nov 25, 2016 at 1:41 AM, Aniroop Mathur wrote:
>> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
>> then SYN_DROPPED event is inserted in the event queue always.
>>
>> However, it is not compulsory that some events are flushed out on every
>> EVIOCG[type] ioctl call like in case of empty event queue and in case when
>> EVIOCG[type] ioctl is issued for say A type of events but event queue does
>> not have any A type of events but some other type of events.
>>
>> Therefore, insert SYN_DROPPED event only when some events have been flushed
>> out from event queue plus bits_to_user fails.
>>
>> Signed-off-by: Aniroop Mathur
>> ---
>> drivers/input/evdev.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..f8b295e 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client
>> *client,
>> }
>>
>> /* flush queued events of type @type, caller must hold client->buffer_lock
>> */
>> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int
>> type)
>> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
>> + unsigned int type)
>> {
>> unsigned int i, head, num;
>> + unsigned int drop_count = 0;
>> unsigned int mask = client->bufsize - 1;
>> bool is_report;
>> struct input_event *ev;
>> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>>
>> if (ev->type == type) {
>> /* drop matched entry */
>> + drop_count++;
>> continue;
>> } else if (is_report && !num) {
>> /* drop empty SYN_REPORT groups */
>> + drop_count++;
>> continue;
>> } else if (head != i) {
>> /* move entry to fill the gap */
>> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>> }
>>
>> client->head = head;
>> + return drop_count;
>> }
>>
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client
>> *client,
>> int ret;
>> unsigned long *mem;
>> size_t len;
>> + unsigned int drop_count = 0;
>>
>> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
>> mem = kmalloc(len, GFP_KERNEL);
>> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
>> *client,
>>
>> spin_unlock(&dev->event_lock);
>>
>> - __evdev_flush_queue(client, type);
>> + drop_count = __evdev_flush_queue(client, type);
>>
>> spin_unlock_irq(&client->buffer_lock);
>>
>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> - if (ret < 0)
>> + if (ret < 0 && drop_count > 0)
>> evdev_queue_syn_dropped(client);
>>
>> kfree(mem);
>> --
>> 2.6.2
>>
Re: [PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
Hello Mr. Torokhov,
Would you kindly update about this patch?
Thanks!
Best Regards,
Aniroop Mathur
On Fri, Nov 25, 2016 at 1:41 AM, Aniroop Mathur wrote:
> Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
> then SYN_DROPPED event is inserted in the event queue always.
>
> However, it is not compulsory that some events are flushed out on every
> EVIOCG[type] ioctl call like in case of empty event queue and in case when
> EVIOCG[type] ioctl is issued for say A type of events but event queue does
> not have any A type of events but some other type of events.
>
> Therefore, insert SYN_DROPPED event only when some events have been flushed
> out from event queue plus bits_to_user fails.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/evdev.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..f8b295e 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client
> *client,
> }
>
> /* flush queued events of type @type, caller must hold client->buffer_lock */
> -static void __evdev_flush_queue(struct evdev_client *client, unsigned int
> type)
> +static unsigned int __evdev_flush_queue(struct evdev_client *client,
> + unsigned int type)
> {
> unsigned int i, head, num;
> + unsigned int drop_count = 0;
> unsigned int mask = client->bufsize - 1;
> bool is_report;
> struct input_event *ev;
> @@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client
> *client, unsigned int type)
>
> if (ev->type == type) {
> /* drop matched entry */
> + drop_count++;
> continue;
> } else if (is_report && !num) {
> /* drop empty SYN_REPORT groups */
> + drop_count++;
> continue;
> } else if (head != i) {
> /* move entry to fill the gap */
> @@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
> *client, unsigned int type)
> }
>
> client->head = head;
> + return drop_count;
> }
>
> static void __evdev_queue_syn_dropped(struct evdev_client *client)
> @@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client
> *client,
> int ret;
> unsigned long *mem;
> size_t len;
> + unsigned int drop_count = 0;
>
> len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> mem = kmalloc(len, GFP_KERNEL);
> @@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
> *client,
>
> spin_unlock(&dev->event_lock);
>
> - __evdev_flush_queue(client, type);
> + drop_count = __evdev_flush_queue(client, type);
>
> spin_unlock_irq(&client->buffer_lock);
>
> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> - if (ret < 0)
> + if (ret < 0 && drop_count > 0)
> evdev_queue_syn_dropped(client);
>
> kfree(mem);
> --
> 2.6.2
>
Re: [PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs
Hello Mr. Igor Grinberg
On Sun, Dec 4, 2016 at 1:32 PM, Igor Grinberg wrote:
> Hi Aniroop Mathur,
>
> On 11/29/16 23:02, Aniroop Mathur wrote:
>> Dear Mike Rapoport, Igor Grinberg,
>> Greetings!
>>
>> I am Aniroop Mathur from Samsung R&D Institute, India.
>>
>> I have submitted one patch as below for review to Linux Open Source.
>> The problem is that we do not have the hardware available with us to
>> test it and we would like to test it before actually applying it.
>> As you are the author of this driver, I am contacting you to request you
>> provide your feedback upon this patch.
>>
>> Also if you have the hardware available, could you please help to
>> test this patch on your hardware? or could you provide contact points
>> of individuals who could support to test it?
>
> This touchpad and the driver was used on an old PXA270 based PDA.
> I currently don't have those at hand to test the patch.
>
>>
>> Thank you!
>>
>> BR,
>> Aniroop Mathur
>>
>> On Tue, Nov 29, 2016 at 1:25 AM, Aniroop Mathur wrote:
>>> msleep(1~20) may not do what the caller intends, and will often sleep
>>> longer.
>>> (~20 ms actual sleep for any value given in the 1~20ms range)
>
> Well, it should be at least 1ms as stated in my comment just before the
> define.
> So, from the correctness perspective larger values will also do the job.
> Additionally, since I've taken a spare 2ms, and you are making it even more
> precise (3000us + 100us) - it will still do the job and stay correct.
> So, there should be no issue from correctness POV.
>
Alright, Thanks!
>>> This is not the desired behaviour for many cases like device resume time,
>>> device suspend time, device enable time, retry logic, etc.
>>> Thus, change msleep to usleep_range for precise wakeups.
>
> This is a human interface touchpad device, even having 20ms soft reset
> sleep time will not impact the responsiveness for humans.
> IMHO, there is no need for precise wakeups for this device, so I wouldn't
> bother.
>
Well, from the point of view of device working and responsiveness for "humans",
I agree that it is okay to sleep for 20 / 40 ms or even 100 ms. However, this
patch is not trying to solve any such issues. This patch is only trying to make
the process sleep for appropriate time as mentioned in the parameter and does
not cause any harm here. I could see that this function is called during device
resume and device probe time. So this change will improve the resume and probe
time for this device and doing the same change in other drivers will
contribute to
improvement in overall kernel resume and boot time a little bit so response time
increases a little bit. Plus, it is recommended and mentioned in kernel
documentation to use usleep_range for delays between 1-10 ms.
So usleep_range should serve better here.
Explained originally here to why not use msleep for 1 - 20 ms:
http://lkml.org/lkml/2007/8/3/250
Thanks.
BR,
Aniroop Mathur
>>>
>>> Signed-off-by: Aniroop Mathur
>>> ---
>>> drivers/input/mouse/synaptics_i2c.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/input/mouse/synaptics_i2c.c
>>> b/drivers/input/mouse/synaptics_i2c.c
>>> index aa7c5da..4d688a6 100644
>>> --- a/drivers/input/mouse/synaptics_i2c.c
>>> +++ b/drivers/input/mouse/synaptics_i2c.c
>>> @@ -29,7 +29,7 @@
>>> * after soft reset, we should wait for 1 ms
>>> * before the device becomes operational
>>> */
>>> -#define SOFT_RESET_DELAY_MS3
>>> +#define SOFT_RESET_DELAY_US3000
>>> /* and after hard reset, we should wait for max 500ms */
>>> #define HARD_RESET_DELAY_MS500
>>>
>>> @@ -311,7 +311,7 @@ static int synaptics_i2c_reset_config(struct i2c_client
>>> *client)
>>> if (ret) {
>>> dev_err(&client->dev, "Unable to reset device\n");
>>> } else {
>>> - msleep(SOFT_RESET_DELAY_MS);
>>> + usleep_range(SOFT_RESET_DELAY_US, SOFT_RESET_DELAY_US +
>>> 100);
>>> ret = synaptics_i2c_config(client);
>>> if (ret)
>>> dev_err(&client->dev, "Unable to config device\n");
>>> --
>>> 2.6.2
>>>
>>
>
> --
> Regards,
> Igor.
Re: [PATCH] Input: touchscreen: edt_ft5x06 - change msleep to usleep_range for small msecs
Hello Mr. Simon, On Sat, Dec 3, 2016 at 10:58 PM, Simon Budig wrote: > Hello Mr, Mathur. > > On 29/11/16 21:54, Aniroop Mathur wrote: >> I have submitted one patch as below for review to Linux Open Source. >> The problem is that we do not have the hardware available with us to >> test it and we would like to test it before actually applying it. >> As you are the author of this driver, I am contacting you to request you to >> provide your feedback upon this patch. >> >> Also if you have the hardware available, could you please help to >> test this patch on your hardware? or could you provide contact points >> of individuals who could support to test it? > > My first question regarding the patch is: What is your motivation for > doing this change? Did you actually encounter any problems with some > touch hardware? Or is this a part of a global search/replace mission > across the linux kernel? > Well firstly, I decided to change this as it is recommended and mentioned in the kernel documentation to use usleep_range over msleep for 1 - 10 ms delays. Secondly, we found problems in response time in our sensor drivers because there is need to give delays after some register initialization to make the device work properly.and when we changed to usleep_range we got decent results like first sensor data was generated faster than before indeed. Since I work on input/iio device drivers so I decided to make same changes in other input subsystem drivers as well. > The change is in a function that is pretty irrelevant for most users of > the driver: reading out raw sensor data, which is only available for > EDT's M06 models of the touchscreen. I am actually tempted to remove > this stuff, since it never really provided helpful for us in debugging > touch screen problems, and adds a certain amount of complexity to the > driver. > > I don't have a setup for the hardware readily available at the moment, > so I can't currently help you there. But from reading the patch it seems > pretty harmless and from my point of view there is nothing that speaks > against incorporating that patch. On the other hand I don't see a lot > that speaks in favor of it. > > *shrug* > I guess, reviewing the patch for this change should be okay. Thanks! Since here we are using msleep(1) which is worse as it sleeps for minimum two jiffies so 20 ms on HZ=100 system and we are doing here 100 retries if register reading fails. So as you can deduce usleep_range will serve better here. Explained originally here why to not use msleep for 1-20ms: http://lkml.org/lkml/2007/8/3/250 BR, Aniroop Mathur > Bye, > Simon > -- > kernel concepts GmbH Simon Budig > Sieghuetter Hauptweg 48 simon.bu...@kernelconcepts.de > D-57072 Siegen +49-271-771091-17 > http://www.kernelconcepts.de/ > HR Siegen, HR B 9613; Geschäftsführer: Nils Faerber, Ole Reinhardt > >
Re: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs
On Tue, Nov 29, 2016 at 12:29 PM, vojt...@ucw.cz wrote: > On Mon, Nov 28, 2016 at 01:49:31PM +0000, Aniroop Mathur wrote: >> Hello Mr. Vojtech Pavlik, >> >> On 28 Nov 2016 17:23, "vojt...@ucw.cz" wrote: >> > >> > Hi. >> > >> > ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer >> > sleep doesn't matter. In the initilization sequence - first chunk of >> > your patch - a way too long delay could in theory make the device fail >> > to initialize. What's critical is that the mdelay() calls are precise. >> > >> > One day I'll open my box of old joystick and re-test these drivers to >> > see if they survived the years of kernel infrastructure updates ... >> > >> > Vojtech >> > >> >> Well, it seems to me that there is some kind of confusion, so I'd like to >> clarify things about this patch. >> As you have mentioned that in the initialization sequence, long delay could >> in theory make the device fail to initialize - This patch actually solves >> this problem. >> msleep is built on jiffies / legacy timers and usleep_range is built on top >> of hrtimers so the wakeup will be precise. >> Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt >> >> For example in initialization sequence, if we use msleep(4), then the >> process >> could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on >> machine architecture. Like on a machine with tick rate / HZ is defined to be >> 100 so msleep(4) will make the process to sleep for minimum 10 ms. >> Whereas usleep_range(4000, 4100) will make sure that the process do not >> sleep >> for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is >> not. >> >> Originally, I added you in this patch to request you if you could help to >> test this patch or provide contact points of individuals who could help >> to test this patch as we do not have the hardware available with us? >> Like this driver, we also need to test other joystick drivers as well like >> gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have >> similar problem. >> Original patch link - https://patchwork.kernel.org/patch/9446201/ >> As we do not have hardware available, so we decided to split patch into >> individual drivers and request to person who could support to test this >> patch >> >> I have also applied the same patch in our driver whose hardware is available >> with me and I found that wake up time became precise indeed and so I >> decided to apply the same fix in other input subsystem drivers as well. > > I do understand what you're trying to achieve. Both ADI_DATA_DELAY and > ADI_INIT_DELAY are specified as minimum delays. Waiting longer doesn't > cause any trouble, so the patch doesn't need to change that. > > In the initialization sequence, it probably doesn't matter either > whether we wait longer, hence the distinction between msleep() and > mdelay() based on positive/negative numbers. The mdelay() needs to be > exact and the msleep() can be longer. How much longer before it disturbs > the init sequence I'm not sure, probably quite a bit. > > The driver was written a long time before hrtimers existed and as such > it was written expecting that msleep() can take a longer time. > Well I agree that waiting longer shall not cause any trouble. However, using usleep_range does not cause any harm here either. In fact, usleep_range is more advantageous to use here as it makes the wake up more precise than before. So we have little improved connection / initialization time than before which is a good thing indeed as it improves response time. Also, same is being used by new device drivers and now some old device drivers have also changed to ulseep_range for 1- 10 ms range. Also, it is recommended and mentioned in the kernel documentation to use usleep_range for 1-10 ms delays. Explained originally here to why not use msleep for 1-20 ms: http://lkml.org/lkml/2007/8/3/250 So how about using usleep_range api for short delays here? > So your patch is most likely not needed, but I should find an ADI device > and see what happens if I make the sleeps in the init sequence much > longer. > So has it been possible so far to check behavior on large sleeps? > It'd also be interesting to see if the mdelay()s could be replaced with > hrtimer-based delays instead, as that would be nicer to the system - if > they can be precise enough. Also, preemption and maybe interrupts should > be disabled around the mdelays I suppose -
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs
On Sat, Dec 3, 2016 at 2:30 PM, Jonathan Cameron wrote: > On 02/12/16 19:07, Aniroop Mathur wrote: >> On Wed, Nov 30, 2016 at 8:13 PM, Aniroop Mathur wrote: >>> On 30 Nov 2016 19:05, "Lars-Peter Clausen" wrote: >>> > >>> > On 11/27/2016 11:51 AM, Jonathan Cameron wrote: >>> > > On 26/11/16 03:47, Aniroop Mathur wrote: >>> > >> msleep(1~20) may not do what the caller intends, and will often sleep >>> longer. >>> > >> (~20 ms actual sleep for any value given in the 1~20ms range) >>> > >> This is not the desired behaviour for many cases like device resume >>> time, >>> > >> device suspend time, device enable time, data reading time, etc. >>> > >> Thus, change msleep to usleep_range for precise wakeups. >>> > >> >>> > >> Signed-off-by: Aniroop Mathur >>> > > As these need individual review by the various original authors and >>> driver maintainers to >>> > > establish the intent of the sleep, it would have been better to have >>> done a series of >>> > > patches (one per driver) with the relevant maintainers cc'd on the >>> ones that they care about. >>> > > >>> > > Most of these are ADI parts looked after by Lars though so perhaps >>> wait for his comments >>> > > before respining. >>> > >>> > I agree with what Jonathan said. For most of these extending the maximum >>> > sleep time a bit further is ok. >>> > >>> >>> Well, its right that sleep a bit further is okay but this patch is not >>> trying >>> to solve any major bug. This patch is only trying to make the wake up more >>> precise than before. So like with msleep(1), process could sleep for 20 ms >>> so this patch only making the making the process to sleep for 1 ms as >>> mentioned in the parameter. So I think, changing to usleep_range is only >>> advantageous and does not cause any harm here. >>> We have also applied the same change in enable/disable/suspend/resume >>> functions in accel, gyro, mag, etc drivers and found decent results like the >>> first sesor data is generated much faster than before so response time >>> increased. This is critical as sensor can run at a rate of 200Hz / 5ms or >>> even more now a days in new devices. We even applied in probe as doing the >>> same in many drivers contribute to a little saving overall in kernel boot >>> up. >>> Also, it is recommended and mentioned in kernel documentation to use >>> usleep_range for 1-10 ms. >>> Sources - >>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt >>> https://lkml.org/lkml/2007/8/3/250 >>> >> >> >> Hello Mr. Jonathan / Mr. Lars / All, >> >> >> Would you kindly update further about this change? > Hi Aniroop, > > Quite a few of us only manage to get to kernel patches once or twice a week > (in my case only on weekends most weeks). > > Anyhow, I've applied this patch as is. I don't necessarily think the change > is that important in the probe paths, but as you said it does little harm. > > So what the heck ;) > > Applied to the togreg branch of iio.git which I'll push out as testing > once I have a net connection (currently on a train). > This sounds great !! Thanks a lot :) BR, Aniroop Mathur > Thanks, > > Jonathan >> >> >>> Thanks. >>> >>> BR, >>> Aniroop Mathur >
Re: Re: [PATCH] IIO: Change msleep to usleep_range for small msecs
On Wed, Nov 30, 2016 at 8:13 PM, Aniroop Mathur wrote: > On 30 Nov 2016 19:05, "Lars-Peter Clausen" wrote: > > > > On 11/27/2016 11:51 AM, Jonathan Cameron wrote: > > > On 26/11/16 03:47, Aniroop Mathur wrote: > > >> msleep(1~20) may not do what the caller intends, and will often sleep > longer. > > >> (~20 ms actual sleep for any value given in the 1~20ms range) > > >> This is not the desired behaviour for many cases like device resume > time, > > >> device suspend time, device enable time, data reading time, etc. > > >> Thus, change msleep to usleep_range for precise wakeups. > > >> > > >> Signed-off-by: Aniroop Mathur > > > As these need individual review by the various original authors and > driver maintainers to > > > establish the intent of the sleep, it would have been better to have > done a series of > > > patches (one per driver) with the relevant maintainers cc'd on the ones > that they care about. > > > > > > Most of these are ADI parts looked after by Lars though so perhaps wait > for his comments > > > before respining. > > > > I agree with what Jonathan said. For most of these extending the maximum > > sleep time a bit further is ok. > > > > Well, its right that sleep a bit further is okay but this patch is not trying > to solve any major bug. This patch is only trying to make the wake up more > precise than before. So like with msleep(1), process could sleep for 20 ms > so this patch only making the making the process to sleep for 1 ms as > mentioned in the parameter. So I think, changing to usleep_range is only > advantageous and does not cause any harm here. > We have also applied the same change in enable/disable/suspend/resume > functions in accel, gyro, mag, etc drivers and found decent results like the > first sesor data is generated much faster than before so response time > increased. This is critical as sensor can run at a rate of 200Hz / 5ms or > even more now a days in new devices. We even applied in probe as doing the > same in many drivers contribute to a little saving overall in kernel boot up. > Also, it is recommended and mentioned in kernel documentation to use > usleep_range for 1-10 ms. > Sources - > https://www.kernel.org/doc/Documentation/timers/timers-howto.txt > https://lkml.org/lkml/2007/8/3/250 > Hello Mr. Jonathan / Mr. Lars / All, Would you kindly update further about this change? > Thanks. > > BR, > Aniroop Mathur
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs
On Wed, Nov 30, 2016 at 5:49 PM, Linus Walleij wrote: > On Sun, Nov 27, 2016 at 11:51 AM, Jonathan Cameron wrote: >> On 26/11/16 03:47, Aniroop Mathur wrote: > > [bmp280.c] > >>> /* Wait to make sure we started up properly */ >>> - mdelay(data->start_up_time); >>> + usleep_range(data->start_up_time, data->start_up_time + 100); >> >> As this in probe I doubt we really care. Could just set it longer to shut >> up the warnings. >> Still would like some input from say Linus on this... > > Hm, I don't think it's a big issue... this works too it just looks overworked. > > On the runtime_resume() path we use msleep() instead which I guess is why > it is not changed in this patch, but they have the same purpose. > I did change msleep to usleep_range in runtime_resume() in bmp280.c as you know resume time is critical indeed. > Yours, > Linus Walleij
RE: RE: RE: [Please Apply][PATCH] Input: misc: bma150: Change msleep to usleep_range for small msecs
Dear Mr. Torokhov, As this patch is now verified, would you please apply it? Thank you! -- Best Regards, Aniroop Mathur Lead Engineer | System 1 - Sensor R&D Samsung Research Institute India - Noida Tel: 0120-671Ext: 4018 Mob: +91 9971865523 Email: a.mat...@samsung.com - Original Message - Sender : ZHANG Xu (BST/ESA3.1) Date : 2016-12-01 17:14 (GMT+5:30) Title : RE: RE: [PATCH] Input: Change msleep to usleep_range for small msecs Dear Aniroop Mathur Got your point. Thank you for your explanation! Best regards Albert (Xu) ZHANG BST/ESA3.1 -Original Message- From: Aniroop Mathur [mailto:a.mat...@samsung.com] Sent: Thursday, December 01, 2016 6:34 PM To: ZHANG Xu (BST/ESA3.1) ; Dmitry Torokhov ; linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Aniroop Mathur ; SAMUEL SEQUEIRA ; Rahul Mahale Subject: RE: RE: [PATCH] Input: Change msleep to usleep_range for small msecs Dear Mr. Albert Zhang, Thank you for your confirmation! Yes, I think usleep_range(2000, 2100) is better than usleep_range(2000, 2000) because delta time will allow the kernel to batch the processes who need to wake up around same time and generate single interrupt to wake up all of them. So this would be beneficial from power saving point of view. -- Best Regards, Aniroop Mathur - Original Message - Sender : ZHANG Xu (BST/ESA3.1) Date : 2016-12-01 11:19 (GMT+5:30) Title : RE: [PATCH] Input: Change msleep to usleep_range for small msecs Hello Aniroop Mathur Thank you for your mail. We have used the usleep_range() function in our new product's driver and the verification result is working. Your patch for bma150 is definitely working for sure. Just one question need your answer. To replace the msleep(2), is usleep_range(2000, 2100) better than usleep_range(2000, 2000) ? Best regards Albert (Xu) ZHANG BST/ESA3.1 -Original Message- From: mathur.anir...@gmail.com [mailto:mathur.anir...@gmail.com] On Behalf Of Aniroop Mathur Sent: Tuesday, November 29, 2016 12:36 AM To: ZHANG Xu (BST/ESA3.1) ; Dmitry Torokhov ; linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Aniroop Mathur ; s.sam...@samsung.com; r.mah...@samsung.com; Aniroop Mathur Subject: Re: [PATCH] Input: Change msleep to usleep_range for small msecs Hello Mr. Albert Zhang, I am Aniroop Mathur from Samsung R&D Institute, India. I have submitted one patch as below for review to Linux Open Source. The problem is that we do not have the hardware available with us to test it and we would like to test it before actually applying it. As you are the author of this driver, so I would like to request you if you could help to test this patch or provide us the contact points of individuals who could support to get this patch tested? Thank you! BR, Aniroop Mathur On Thu, Nov 24, 2016 at 9:33 PM, Aniroop Mathur wrote: > msleep(1~20) may not do what the caller intends, and will often sleep longer. > (~20 ms actual sleep for any value given in the 1~20ms range) > This is not the desired behaviour for many cases like device resume time, > device suspend time, device enable time, etc. > Thus, change msleep to usleep_range for precise wakeups. > > Signed-off-by: Aniroop Mathur > --- > drivers/input/misc/bma150.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c > index 2124390..1fa8537 100644 > --- a/drivers/input/misc/bma150.c > +++ b/drivers/input/misc/bma150.c > @@ -207,7 +207,7 @@ static int bma150_set_mode(struct bma150_data *bma150, u8 >mode) > return error; > > if (mode == BMA150_MODE_NORMAL) > - msleep(2); > + usleep_range(2000, 2100); > > bma150->mode = mode; > return 0; > @@ -222,7 +222,7 @@ static int bma150_soft_reset(struct bma150_data *bma150) > if (error) > return error; > > - msleep(2); > + usleep_range(2000, 2100); > return 0; > } > > -- > 2.6.2 >
RE: RE: [PATCH] Input: Change msleep to usleep_range for small msecs
Dear Mr. Albert Zhang, Thank you for your confirmation! Yes, I think usleep_range(2000, 2100) is better than usleep_range(2000, 2000) because delta time will allow the kernel to batch the processes who need to wake up around same time and generate single interrupt to wake up all of them. So this would be beneficial from power saving point of view. -- Best Regards, Aniroop Mathur - Original Message - Sender : ZHANG Xu (BST/ESA3.1) Date : 2016-12-01 11:19 (GMT+5:30) Title : RE: [PATCH] Input: Change msleep to usleep_range for small msecs Hello Aniroop Mathur Thank you for your mail. We have used the usleep_range() function in our new product's driver and the verification result is working. Your patch for bma150 is definitely working for sure. Just one question need your answer. To replace the msleep(2), is usleep_range(2000, 2100) better than usleep_range(2000, 2000) ? Best regards Albert (Xu) ZHANG BST/ESA3.1 -Original Message- From: mathur.anir...@gmail.com [mailto:mathur.anir...@gmail.com] On Behalf Of Aniroop Mathur Sent: Tuesday, November 29, 2016 12:36 AM To: ZHANG Xu (BST/ESA3.1) ; Dmitry Torokhov ; linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org Cc: Aniroop Mathur ; s.sam...@samsung.com; r.mah...@samsung.com; Aniroop Mathur Subject: Re: [PATCH] Input: Change msleep to usleep_range for small msecs Hello Mr. Albert Zhang, I am Aniroop Mathur from Samsung R&D Institute, India. I have submitted one patch as below for review to Linux Open Source. The problem is that we do not have the hardware available with us to test it and we would like to test it before actually applying it. As you are the author of this driver, so I would like to request you if you could help to test this patch or provide us the contact points of individuals who could support to get this patch tested? Thank you! BR, Aniroop Mathur On Thu, Nov 24, 2016 at 9:33 PM, Aniroop Mathur wrote: > msleep(1~20) may not do what the caller intends, and will often sleep longer. > (~20 ms actual sleep for any value given in the 1~20ms range) > This is not the desired behaviour for many cases like device resume time, > device suspend time, device enable time, etc. > Thus, change msleep to usleep_range for precise wakeups. > > Signed-off-by: Aniroop Mathur > --- > drivers/input/misc/bma150.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c > index 2124390..1fa8537 100644 > --- a/drivers/input/misc/bma150.c > +++ b/drivers/input/misc/bma150.c > @@ -207,7 +207,7 @@ static int bma150_set_mode(struct bma150_data *bma150, u8 >mode) > return error; > > if (mode == BMA150_MODE_NORMAL) > - msleep(2); > + usleep_range(2000, 2100); > > bma150->mode = mode; > return 0; > @@ -222,7 +222,7 @@ static int bma150_soft_reset(struct bma150_data *bma150) > if (error) > return error; > > - msleep(2); > + usleep_range(2000, 2100); > return 0; > } > > -- > 2.6.2 >
RE: Re: [PATCH] IIO: Change msleep to usleep_range for small msecs
On 30 Nov 2016 19:05, "Lars-Peter Clausen" wrote: > > On 11/27/2016 11:51 AM, Jonathan Cameron wrote: > > On 26/11/16 03:47, Aniroop Mathur wrote: > >> msleep(1~20) may not do what the caller intends, and will often sleep > >> longer. > >> (~20 ms actual sleep for any value given in the 1~20ms range) > >> This is not the desired behaviour for many cases like device resume time, > >> device suspend time, device enable time, data reading time, etc. > >> Thus, change msleep to usleep_range for precise wakeups. > >> > >> Signed-off-by: Aniroop Mathur > > As these need individual review by the various original authors and driver > > maintainers to > > establish the intent of the sleep, it would have been better to have done > > a series of > > patches (one per driver) with the relevant maintainers cc'd on the ones > > that they care about. > > > > Most of these are ADI parts looked after by Lars though so perhaps wait > > for his comments > > before respining. > > I agree with what Jonathan said. For most of these extending the maximum > sleep time a bit further is ok. > Well, its right that sleep a bit further is okay but this patch is not trying to solve any major bug. This patch is only trying to make the wake up more precise than before. So like with msleep(1), process could sleep for 20 ms so this patch only making the making the process to sleep for 1 ms as mentioned in the parameter. So I think, changing to usleep_range is only advantageous and does not cause any harm here. We have also applied the same change in enable/disable/suspend/resume functions in accel, gyro, mag, etc drivers and found decent results like the first sesor data is generated much faster than before so response time increased. This is critical as sensor can run at a rate of 200Hz / 5ms or even more now a days in new devices. We even applied in probe as doing the same in many drivers contribute to a little saving overall in kernel boot up. Also, it is recommended and mentioned in kernel documentation to use usleep_range for 1-10 ms. Sources - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt https://lkml.org/lkml/2007/8/3/250 Thanks. BR, Aniroop Mathur
Re: [PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs
Dear Mike Rapoport, Igor Grinberg,
Greetings!
I am Aniroop Mathur from Samsung R&D Institute, India.
I have submitted one patch as below for review to Linux Open Source.
The problem is that we do not have the hardware available with us to
test it and we would like to test it before actually applying it.
As you are the author of this driver, I am contacting you to request you
provide your feedback upon this patch.
Also if you have the hardware available, could you please help to
test this patch on your hardware? or could you provide contact points
of individuals who could support to test it?
Thank you!
BR,
Aniroop Mathur
On Tue, Nov 29, 2016 at 1:25 AM, Aniroop Mathur wrote:
> msleep(1~20) may not do what the caller intends, and will often sleep longer.
> (~20 ms actual sleep for any value given in the 1~20ms range)
> This is not the desired behaviour for many cases like device resume time,
> device suspend time, device enable time, retry logic, etc.
> Thus, change msleep to usleep_range for precise wakeups.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/mouse/synaptics_i2c.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics_i2c.c
> b/drivers/input/mouse/synaptics_i2c.c
> index aa7c5da..4d688a6 100644
> --- a/drivers/input/mouse/synaptics_i2c.c
> +++ b/drivers/input/mouse/synaptics_i2c.c
> @@ -29,7 +29,7 @@
> * after soft reset, we should wait for 1 ms
> * before the device becomes operational
> */
> -#define SOFT_RESET_DELAY_MS3
> +#define SOFT_RESET_DELAY_US3000
> /* and after hard reset, we should wait for max 500ms */
> #define HARD_RESET_DELAY_MS500
>
> @@ -311,7 +311,7 @@ static int synaptics_i2c_reset_config(struct i2c_client
> *client)
> if (ret) {
> dev_err(&client->dev, "Unable to reset device\n");
> } else {
> - msleep(SOFT_RESET_DELAY_MS);
> + usleep_range(SOFT_RESET_DELAY_US, SOFT_RESET_DELAY_US + 100);
> ret = synaptics_i2c_config(client);
> if (ret)
> dev_err(&client->dev, "Unable to config device\n");
> --
> 2.6.2
>
Re: [PATCH] Input: touchscreen: zylonite_wm97xx - change msleep to usleep_range for small msecs
Dear Mark Brown / Ian Molton / Andrew Zabolotny,
Greetings!
I am Aniroop Mathur from Samsung R&D Institute, India.
I have submitted one patch as below for review to Linux Open Source.
The problem is that we do not have the hardware available with us to
test it and we would like to test it before actually applying it.
As you are the author of this driver, I am contacting you to request you
provide your feedback upon this patch.
Also if you have the hardware available, could you please help to
test this patch on your hardware? or could you provide contact points
of individuals who could support to test it?
Thank you!
BR,
Aniroop Mathur
On Tue, Nov 29, 2016 at 12:14 AM, Aniroop Mathur wrote:
> msleep(1~20) may not do what the caller intends, and will often sleep longer.
> (~20 ms actual sleep for any value given in the 1~20ms range)
> This is not the desired behaviour for many cases like device resume time,
> device suspend time, device enable time, retry logic, etc.
> Thus, change msleep to usleep_range for precise wakeups.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/touchscreen/zylonite-wm97xx.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/zylonite-wm97xx.c
> b/drivers/input/touchscreen/zylonite-wm97xx.c
> index e2ccd68..cebd3a8 100644
> --- a/drivers/input/touchscreen/zylonite-wm97xx.c
> +++ b/drivers/input/touchscreen/zylonite-wm97xx.c
> @@ -81,7 +81,7 @@ static void wm97xx_acc_pen_up(struct wm97xx *wm)
> {
> int i;
>
> - msleep(1);
> + usleep_range(1000, 1100);
>
> for (i = 0; i < 16; i++)
> MODR;
> @@ -98,7 +98,7 @@ static int wm97xx_acc_pen_down(struct wm97xx *wm)
> * for samples. The controller can't have a suitably low
> * threshold set to use the notifications it gives.
> */
> - msleep(1);
> + usleep_range(1000, 1100);
>
> if (tries > 5) {
> tries = 0;
> --
> 2.6.2
>
Re: [PATCH] Input: touchscreen: w90p910 - change msleep to usleep_range for small msecs
Dear Wan ZongShun, Greetings! I am Aniroop Mathur from Samsung R&D Institute, India. I have submitted one patch as below for review to Linux Open Source. The problem is that we do not have the hardware available with us to test it and we would like to test it before actually applying it. As you are the author of this driver, I am contacting you to request you provide your feedback upon this patch. Also if you have the hardware available, could you please help to test this patch on your hardware? or could you provide contact points of individuals who could support to test it? Thank you! BR, Aniroop Mathur On Tue, Nov 29, 2016 at 12:13 AM, Aniroop Mathur wrote: > msleep(1~20) may not do what the caller intends, and will often sleep longer. > (~20 ms actual sleep for any value given in the 1~20ms range) > This is not the desired behaviour for many cases like device resume time, > device suspend time, device enable time, retry logic, etc. > Thus, change msleep to usleep_range for precise wakeups. > > Signed-off-by: Aniroop Mathur > --- > drivers/input/touchscreen/w90p910_ts.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/touchscreen/w90p910_ts.c > b/drivers/input/touchscreen/w90p910_ts.c > index da6004e..3f14b5a 100644 > --- a/drivers/input/touchscreen/w90p910_ts.c > +++ b/drivers/input/touchscreen/w90p910_ts.c > @@ -171,9 +171,9 @@ static int w90p910_open(struct input_dev *dev) > clk_enable(w90p910_ts->clk); > > __raw_writel(ADC_RST1, w90p910_ts->ts_reg); > - msleep(1); > + usleep_range(1000, 1100); > __raw_writel(ADC_RST0, w90p910_ts->ts_reg); > - msleep(1); > + usleep_range(1000, 1100); > > /* set delay and screen type */ > val = __raw_readl(w90p910_ts->ts_reg + 0x04); > -- > 2.6.2 >
Re: [PATCH] Input: touchscreen: edt_ft5x06 - change msleep to usleep_range for small msecs
Dear Simon Budig / Daniel Wagener / Lothar Waßmann,
Greetings!
I am Aniroop Mathur from Samsung R&D Institute, India.
I have submitted one patch as below for review to Linux Open Source.
The problem is that we do not have the hardware available with us to
test it and we would like to test it before actually applying it.
As you are the author of this driver, I am contacting you to request you to
provide your feedback upon this patch.
Also if you have the hardware available, could you please help to
test this patch on your hardware? or could you provide contact points
of individuals who could support to test it?
Thank you!
BR,
Aniroop Mathur
On Tue, Nov 29, 2016 at 12:11 AM, Aniroop Mathur wrote:
> msleep(1~20) may not do what the caller intends, and will often sleep longer.
> (~20 ms actual sleep for any value given in the 1~20ms range)
> This is not the desired behaviour for many cases like device resume time,
> device suspend time, device enable time, retry logic, etc.
> Thus, change msleep to usleep_range for precise wakeups.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/touchscreen/edt-ft5x06.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/edt-ft5x06.c
> b/drivers/input/touchscreen/edt-ft5x06.c
> index 703e295..379dd31 100644
> --- a/drivers/input/touchscreen/edt-ft5x06.c
> +++ b/drivers/input/touchscreen/edt-ft5x06.c
> @@ -67,7 +67,7 @@
> #define EDT_SWITCH_MODE_RETRIES10
> #define EDT_SWITCH_MODE_DELAY 5 /* msec */
> #define EDT_RAW_DATA_RETRIES 100
> -#define EDT_RAW_DATA_DELAY 1 /* msec */
> +#define EDT_RAW_DATA_DELAY 1000 /* usec */
>
> enum edt_ver {
> M06,
> @@ -664,7 +664,7 @@ static ssize_t edt_ft5x06_debugfs_raw_data_read(struct
> file *file,
> }
>
> do {
> - msleep(EDT_RAW_DATA_DELAY);
> + usleep_range(EDT_RAW_DATA_DELAY, EDT_RAW_DATA_DELAY + 100);
> val = edt_ft5x06_register_read(tsdata, 0x08);
> if (val < 1)
> break;
> --
> 2.6.2
>
Re: [PATCH] Input: mouse: pxa930_trkball - change msleep to usleep_range for small msecs
Dear Yong Yao,
Greetings!
I am Aniroop Mathur from Samsung R&D Institute, India.
I have submitted one patch as below for review to Linux Open Source.
The problem is that we do not have the hardware available with us to
test it and we would like to test it before actually applying it.
As you are the author of this driver, I am contacting you to request you
provide your feedback upon this patch.
Also if you have the hardware available, could you please help to
test this patch on your hardware? or could you provide contact points
of individuals who could support to test it?
Thank you!
BR,
Aniroop Mathur
On Mon, Nov 28, 2016 at 10:59 PM, Aniroop Mathur wrote:
> msleep(1~20) may not do what the caller intends, and will often sleep longer.
> (~20 ms actual sleep for any value given in the 1~20ms range)
> This is not the desired behaviour for many cases like device resume time,
> device suspend time, device enable time, retry logic, etc.
> Thus, change msleep to usleep_range for precise wakeups.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/mouse/pxa930_trkball.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/pxa930_trkball.c
> b/drivers/input/mouse/pxa930_trkball.c
> index 9b4d9a5..d8ac9f9 100644
> --- a/drivers/input/mouse/pxa930_trkball.c
> +++ b/drivers/input/mouse/pxa930_trkball.c
> @@ -85,7 +85,7 @@ static int write_tbcr(struct pxa930_trkball *trkball, int v)
> while (--i) {
> if (__raw_readl(trkball->mmio_base + TBCR) == v)
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
>
> if (i == 0) {
> --
> 2.6.2
>
Re: [PATCH] Input: mouse: navpoint - Change msleep to usleep_range for small msecs
Dear Mr. Paul Parsons,
I am Aniroop Mathur from Samsung R&D Institute, India.
I have submitted one patch as below for review to Linux Open Source.
The problem is that we do not have the hardware available with us to
test it and we would like to test it before actually applying it.
If you have the hardware available, could you please help to test this patch?
or could you provide contact points of individuals who could support to test it?
Thank you!
BR,
Aniroop Mathur
On Mon, Nov 28, 2016 at 10:58 PM, Aniroop Mathur wrote:
> msleep(1~20) may not do what the caller intends, and will often sleep longer.
> (~20 ms actual sleep for any value given in the 1~20ms range)
> This is not the desired behaviour for many cases like device resume time,
> device suspend time, device enable time, retry logic, etc.
> Thus, change msleep to usleep_range for precise wakeups.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/mouse/navpoint.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/navpoint.c b/drivers/input/mouse/navpoint.c
> index d6e8f58..2ede00b 100644
> --- a/drivers/input/mouse/navpoint.c
> +++ b/drivers/input/mouse/navpoint.c
> @@ -166,7 +166,7 @@ static void navpoint_up(struct navpoint *navpoint)
> for (timeout = 100; timeout != 0; --timeout) {
> if (!(pxa_ssp_read_reg(ssp, SSSR) & SSSR_CSS))
> break;
> - msleep(1);
> + usleep_range(1000, 1100);
> }
>
> if (timeout == 0)
> --
> 2.6.2
>
[PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/mouse/synaptics_i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/synaptics_i2c.c
b/drivers/input/mouse/synaptics_i2c.c
index aa7c5da..4d688a6 100644
--- a/drivers/input/mouse/synaptics_i2c.c
+++ b/drivers/input/mouse/synaptics_i2c.c
@@ -29,7 +29,7 @@
* after soft reset, we should wait for 1 ms
* before the device becomes operational
*/
-#define SOFT_RESET_DELAY_MS3
+#define SOFT_RESET_DELAY_US3000
/* and after hard reset, we should wait for max 500ms */
#define HARD_RESET_DELAY_MS500
@@ -311,7 +311,7 @@ static int synaptics_i2c_reset_config(struct i2c_client
*client)
if (ret) {
dev_err(&client->dev, "Unable to reset device\n");
} else {
- msleep(SOFT_RESET_DELAY_MS);
+ usleep_range(SOFT_RESET_DELAY_US, SOFT_RESET_DELAY_US + 100);
ret = synaptics_i2c_config(client);
if (ret)
dev_err(&client->dev, "Unable to config device\n");
--
2.6.2
[PATCH] Input: joystick: gf2k - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, connection time, probe time,
loops, retry logic, etc
msleep is built on jiffies / legacy timers which are not precise whereas
usleep_range is build on top of hrtimers so the wakeups are precise.
Thus, change msleep to usleep_range for precise wakeups.
For example:
On a machine with tick rate / HZ as 100, msleep(4) will make the process to
sleep for a minimum period of 10 ms whereas usleep_range(4000, 4100) will make
sure that the process does not sleep for more than 4100 us or 4.1ms
Signed-off-by: Aniroop Mathur
---
drivers/input/joystick/gf2k.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/input/joystick/gf2k.c b/drivers/input/joystick/gf2k.c
index 0f519db..e9d5095 100644
--- a/drivers/input/joystick/gf2k.c
+++ b/drivers/input/joystick/gf2k.c
@@ -42,7 +42,7 @@ MODULE_LICENSE("GPL");
#define GF2K_START 400 /* The time we wait for the first bit
[400 us] */
#define GF2K_STROBE40 /* The time we wait for the first bit
[40 us] */
-#define GF2K_TIMEOUT 4 /* Wait for everything to settle [4 ms]
*/
+#define GF2K_TIMEOUT 4000/* Wait for everything to settle [4000
us] */
#define GF2K_LENGTH80 /* Max number of triplets in a packet */
/*
@@ -138,7 +138,7 @@ static void gf2k_trigger_seq(struct gameport *gameport,
short *seq)
i = 0;
do {
gameport_trigger(gameport);
- t = gameport_time(gameport, GF2K_TIMEOUT * 1000);
+ t = gameport_time(gameport, GF2K_TIMEOUT);
while ((gameport_read(gameport) & 1) && t) t--;
udelay(seq[i]);
} while (seq[++i]);
@@ -259,11 +259,11 @@ static int gf2k_connect(struct gameport *gameport, struct
gameport_driver *drv)
gf2k_trigger_seq(gameport, gf2k_seq_reset);
- msleep(GF2K_TIMEOUT);
+ usleep_range(GF2K_TIMEOUT, GF2K_TIMEOUT + 100);
gf2k_trigger_seq(gameport, gf2k_seq_digital);
- msleep(GF2K_TIMEOUT);
+ usleep_range(GF2K_TIMEOUT, GF2K_TIMEOUT + 100);
if (gf2k_read_packet(gameport, GF2K_LENGTH, data) < 12) {
err = -ENODEV;
--
2.6.2
[PATCH] Input: joystick: analog - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, connection time, probe time,
loops, retry logic, etc
msleep is built on jiffies / legacy timers which are not precise whereas
usleep_range is build on top of hrtimers so the wakeups are precise.
Thus, change msleep to usleep_range for precise wakeups.
For example:
On a machine with tick rate / HZ as 100, msleep(3) will make the process to
sleep for a minimum period of 10 ms whereas usleep_range(3000, 3100) will make
sure that the process does not sleep for more than 3100 us or 3.1ms
Signed-off-by: Aniroop Mathur
---
drivers/input/joystick/analog.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 3d8ff09..2891704 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -88,7 +88,7 @@ MODULE_PARM_DESC(map, "Describes analog joysticks
type/capabilities");
#define ANALOG_EXTENSIONS 0x7ff00
#define ANALOG_GAMEPAD 0x8
-#define ANALOG_MAX_TIME3 /* 3 ms */
+#define ANALOG_MAX_TIME3000/* 3000 us */
#define ANALOG_LOOP_TIME 2000/* 2 * loop */
#define ANALOG_SAITEK_DELAY200 /* 200 us */
#define ANALOG_SAITEK_TIME 2000/* 2000 us */
@@ -257,7 +257,7 @@ static int analog_cooked_read(struct analog_port *port)
int i, j;
loopout = (ANALOG_LOOP_TIME * port->loop) / 1000;
- timeout = ANALOG_MAX_TIME * port->speed;
+ timeout = (ANALOG_MAX_TIME / 1000) * port->speed;
local_irq_save(flags);
gameport_trigger(gameport);
@@ -625,20 +625,20 @@ static int analog_init_port(struct gameport *gameport,
struct gameport_driver *d
gameport_trigger(gameport);
t = gameport_read(gameport);
- msleep(ANALOG_MAX_TIME);
+ usleep_range(ANALOG_MAX_TIME, ANALOG_MAX_TIME + 100);
port->mask = (gameport_read(gameport) ^ t) & t & 0xf;
port->fuzz = (port->speed * ANALOG_FUZZ_MAGIC) / port->loop /
1000 + ANALOG_FUZZ_BITS;
for (i = 0; i < ANALOG_INIT_RETRIES; i++) {
if (!analog_cooked_read(port))
break;
- msleep(ANALOG_MAX_TIME);
+ usleep_range(ANALOG_MAX_TIME, ANALOG_MAX_TIME + 100);
}
u = v = 0;
- msleep(ANALOG_MAX_TIME);
- t = gameport_time(gameport, ANALOG_MAX_TIME * 1000);
+ usleep_range(ANALOG_MAX_TIME, ANALOG_MAX_TIME + 100);
+ t = gameport_time(gameport, ANALOG_MAX_TIME);
gameport_trigger(gameport);
while ((gameport_read(port->gameport) & port->mask) && (u < t))
u++;
--
2.6.2
[PATCH] Input: joystick: sidewinder - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, connection time, probe time,
loops, retry logic, etc
msleep is built on jiffies / legacy timers which are not precise whereas
usleep_range is build on top of hrtimers so the wakeups are precise.
Thus, change msleep to usleep_range for precise wakeups.
For example:
On a machine with tick rate / HZ as 100, msleep(6) will make the process to
sleep for a minimum period of 10 ms whereas usleep_range(6000, 6100) will make
sure that the process does not sleep for more than 6100 us or 6.1ms
Signed-off-by: Aniroop Mathur
---
drivers/input/joystick/sidewinder.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/input/joystick/sidewinder.c
b/drivers/input/joystick/sidewinder.c
index 4a95b22..e5a1292 100644
--- a/drivers/input/joystick/sidewinder.c
+++ b/drivers/input/joystick/sidewinder.c
@@ -50,7 +50,7 @@ MODULE_LICENSE("GPL");
#define SW_START 600 /* The time we wait for the first bit [600 us]
*/
#define SW_STROBE 60 /* Max time per bit [60 us] */
-#define SW_TIMEOUT 6 /* Wait for everything to settle [6 ms] */
+#define SW_TIMEOUT 6000/* Wait for everything to settle [6000 us] */
#define SW_KICK45 /* Wait after A0 fall till kick [45 us]
*/
#define SW_END 8 /* Number of bits before end of packet to kick
*/
#define SW_FAIL16 /* Number of packet read errors to fail
and reinitialize */
@@ -139,7 +139,7 @@ static int sw_read_packet(struct gameport *gameport,
unsigned char *buf, int len
unsigned char pending, u, v;
i = -id;/* Don't care
about data, only want ID */
- timeout = id ? gameport_time(gameport, SW_TIMEOUT * 1000) : 0; /* Set
up global timeout for ID packet */
+ timeout = id ? gameport_time(gameport, SW_TIMEOUT) : 0; /* Set up
global timeout for ID packet */
kick = id ? gameport_time(gameport, SW_KICK) : 0; /* Set up kick
timeout for ID packet */
start = gameport_time(gameport, SW_START);
strobe = gameport_time(gameport, SW_STROBE);
@@ -248,7 +248,7 @@ static void sw_init_digital(struct gameport *gameport)
i = 0;
do {
gameport_trigger(gameport);/* Trigger */
- t = gameport_time(gameport, SW_TIMEOUT * 1000);
+ t = gameport_time(gameport, SW_TIMEOUT);
while ((gameport_read(gameport) & 1) && t) t--; /* Wait for
axis to fall back to 0 */
udelay(seq[i]);/*
Delay magic time */
} while (seq[++i]);
@@ -483,13 +483,13 @@ static int sw_read(struct sw *sw)
" - reinitializing joystick.\n", sw->gameport->phys);
if (!i && sw->type == SW_ID_3DP) {
/* 3D Pro can be in analog mode */
- mdelay(3 * SW_TIMEOUT);
+ mdelay(3 * (SW_TIMEOUT / 1000));
sw_init_digital(sw->gameport);
}
- mdelay(SW_TIMEOUT);
+ mdelay(SW_TIMEOUT / 1000);
i = sw_read_packet(sw->gameport, buf, SW_LENGTH, 0);
/* Read normal data packet */
- mdelay(SW_TIMEOUT);
+ mdelay(SW_TIMEOUT / 1000);
sw_read_packet(sw->gameport, buf, SW_LENGTH, i);
/* Read ID packet, this initializes the stick */
sw->fail = SW_FAIL;
@@ -616,14 +616,14 @@ static int sw_connect(struct gameport *gameport, struct
gameport_driver *drv)
gameport->phys, gameport->io, gameport->speed);
i = sw_read_packet(gameport, buf, SW_LENGTH, 0);/* Read
normal packet */
- msleep(SW_TIMEOUT);
+ usleep_range(SW_TIMEOUT, SW_TIMEOUT + 100);
dbg("Init 1: Mode %d. Length %d.", m , i);
if (!i) { /* No
data. 3d Pro analog mode? */
sw_init_digital(gameport); /*
Switch to digital */
- msleep(SW_TIMEOUT);
+ usleep_range(SW_TIMEOUT, SW_TIMEOUT + 100);
i = sw_read_packet(gameport, buf, SW_LENGTH, 0);/*
Retry reading packet */
- msleep(SW_TIMEOUT);
+ usleep_range(SW_TIMEOUT, SW_TIMEOUT + 100);
dbg("Init 1b: Length %d.", i);
if (!i) { /* No
data -> FAIL */
err = -ENODEV;
@@ -636,7 +636,7 @@ static int sw_connect(struct gameport *gameport, struc
[PATCH] Input: touchscreen: zylonite_wm97xx - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/touchscreen/zylonite-wm97xx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/zylonite-wm97xx.c
b/drivers/input/touchscreen/zylonite-wm97xx.c
index e2ccd68..cebd3a8 100644
--- a/drivers/input/touchscreen/zylonite-wm97xx.c
+++ b/drivers/input/touchscreen/zylonite-wm97xx.c
@@ -81,7 +81,7 @@ static void wm97xx_acc_pen_up(struct wm97xx *wm)
{
int i;
- msleep(1);
+ usleep_range(1000, 1100);
for (i = 0; i < 16; i++)
MODR;
@@ -98,7 +98,7 @@ static int wm97xx_acc_pen_down(struct wm97xx *wm)
* for samples. The controller can't have a suitably low
* threshold set to use the notifications it gives.
*/
- msleep(1);
+ usleep_range(1000, 1100);
if (tries > 5) {
tries = 0;
--
2.6.2
[PATCH] Input: touchscreen: w90p910 - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer. (~20 ms actual sleep for any value given in the 1~20ms range) This is not the desired behaviour for many cases like device resume time, device suspend time, device enable time, retry logic, etc. Thus, change msleep to usleep_range for precise wakeups. Signed-off-by: Aniroop Mathur --- drivers/input/touchscreen/w90p910_ts.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/input/touchscreen/w90p910_ts.c b/drivers/input/touchscreen/w90p910_ts.c index da6004e..3f14b5a 100644 --- a/drivers/input/touchscreen/w90p910_ts.c +++ b/drivers/input/touchscreen/w90p910_ts.c @@ -171,9 +171,9 @@ static int w90p910_open(struct input_dev *dev) clk_enable(w90p910_ts->clk); __raw_writel(ADC_RST1, w90p910_ts->ts_reg); - msleep(1); + usleep_range(1000, 1100); __raw_writel(ADC_RST0, w90p910_ts->ts_reg); - msleep(1); + usleep_range(1000, 1100); /* set delay and screen type */ val = __raw_readl(w90p910_ts->ts_reg + 0x04); -- 2.6.2
[PATCH] Input: touchscreen: edt_ft5x06 - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/touchscreen/edt-ft5x06.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/edt-ft5x06.c
b/drivers/input/touchscreen/edt-ft5x06.c
index 703e295..379dd31 100644
--- a/drivers/input/touchscreen/edt-ft5x06.c
+++ b/drivers/input/touchscreen/edt-ft5x06.c
@@ -67,7 +67,7 @@
#define EDT_SWITCH_MODE_RETRIES10
#define EDT_SWITCH_MODE_DELAY 5 /* msec */
#define EDT_RAW_DATA_RETRIES 100
-#define EDT_RAW_DATA_DELAY 1 /* msec */
+#define EDT_RAW_DATA_DELAY 1000 /* usec */
enum edt_ver {
M06,
@@ -664,7 +664,7 @@ static ssize_t edt_ft5x06_debugfs_raw_data_read(struct file
*file,
}
do {
- msleep(EDT_RAW_DATA_DELAY);
+ usleep_range(EDT_RAW_DATA_DELAY, EDT_RAW_DATA_DELAY + 100);
val = edt_ft5x06_register_read(tsdata, 0x08);
if (val < 1)
break;
--
2.6.2
[PATCH] Input: touchscreen: ads7846 - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/touchscreen/ads7846.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index 1ce3ecb..b1a5a6c 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -58,7 +58,7 @@
* files.
*/
-#define TS_POLL_DELAY 1 /* ms delay before the first sample */
+#define TS_POLL_DELAY 1000/* us delay before the first sample */
#define TS_POLL_PERIOD 5 /* ms delay between samples */
/* this driver doesn't aim at the peak continuous sample rate */
@@ -857,7 +857,7 @@ static irqreturn_t ads7846_irq(int irq, void *handle)
struct ads7846 *ts = handle;
/* Start with a small delay before checking pendown state */
- msleep(TS_POLL_DELAY);
+ usleep_range(TS_POLL_DELAY, TS_POLL_DELAY + 100);
while (!ts->stopped && get_pendown_state(ts)) {
--
2.6.2
[PATCH] Input: mouse: synaptics - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/mouse/synaptics_i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/synaptics_i2c.c
b/drivers/input/mouse/synaptics_i2c.c
index aa7c5da..4d688a6 100644
--- a/drivers/input/mouse/synaptics_i2c.c
+++ b/drivers/input/mouse/synaptics_i2c.c
@@ -29,7 +29,7 @@
* after soft reset, we should wait for 1 ms
* before the device becomes operational
*/
-#define SOFT_RESET_DELAY_MS3
+#define SOFT_RESET_DELAY_US3000
/* and after hard reset, we should wait for max 500ms */
#define HARD_RESET_DELAY_MS500
@@ -311,7 +311,7 @@ static int synaptics_i2c_reset_config(struct i2c_client
*client)
if (ret) {
dev_err(&client->dev, "Unable to reset device\n");
} else {
- msleep(SOFT_RESET_DELAY_MS);
+ usleep_range(SOFT_RESET_DELAY_MS, SOFT_RESET_DELAY_MS + 100);
ret = synaptics_i2c_config(client);
if (ret)
dev_err(&client->dev, "Unable to config device\n");
--
2.6.2
[PATCH] Input: mouse: pxa930_trkball - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/mouse/pxa930_trkball.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/pxa930_trkball.c
b/drivers/input/mouse/pxa930_trkball.c
index 9b4d9a5..d8ac9f9 100644
--- a/drivers/input/mouse/pxa930_trkball.c
+++ b/drivers/input/mouse/pxa930_trkball.c
@@ -85,7 +85,7 @@ static int write_tbcr(struct pxa930_trkball *trkball, int v)
while (--i) {
if (__raw_readl(trkball->mmio_base + TBCR) == v)
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
if (i == 0) {
--
2.6.2
[PATCH] Input: mouse: navpoint - Change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, retry logic, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/mouse/navpoint.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/navpoint.c b/drivers/input/mouse/navpoint.c
index d6e8f58..2ede00b 100644
--- a/drivers/input/mouse/navpoint.c
+++ b/drivers/input/mouse/navpoint.c
@@ -166,7 +166,7 @@ static void navpoint_up(struct navpoint *navpoint)
for (timeout = 100; timeout != 0; --timeout) {
if (!(pxa_ssp_read_reg(ssp, SSSR) & SSSR_CSS))
break;
- msleep(1);
+ usleep_range(1000, 1100);
}
if (timeout == 0)
--
2.6.2
[PATCH] Input: keyboard: lm8323 - Change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer. (~20 ms actual sleep for any value given in the 1~20ms range) This is not the desired behaviour for many cases like device resume time, device suspend time, device enable time, retry logic, etc. Thus, change msleep to usleep_range for precise wakeups. Signed-off-by: Aniroop Mathur --- drivers/input/keyboard/lm8323.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c index 21bea52..14679e9 100644 --- a/drivers/input/keyboard/lm8323.c +++ b/drivers/input/keyboard/lm8323.c @@ -689,7 +689,7 @@ static int lm8323_probe(struct i2c_client *client, break; } - msleep(1); + usleep_range(1000, 1100); } lm8323_configure(lm); -- 2.6.2
Re: [PATCH] Input: Change msleep to usleep_range for small msecs
Hello Mr. Albert Zhang, I am Aniroop Mathur from Samsung R&D Institute, India. I have submitted one patch as below for review to Linux Open Source. The problem is that we do not have the hardware available with us to test it and we would like to test it before actually applying it. As you are the author of this driver, so I would like to request you if you could help to test this patch or provide us the contact points of individuals who could support to get this patch tested? Thank you! BR, Aniroop Mathur On Thu, Nov 24, 2016 at 9:33 PM, Aniroop Mathur wrote: > msleep(1~20) may not do what the caller intends, and will often sleep longer. > (~20 ms actual sleep for any value given in the 1~20ms range) > This is not the desired behaviour for many cases like device resume time, > device suspend time, device enable time, etc. > Thus, change msleep to usleep_range for precise wakeups. > > Signed-off-by: Aniroop Mathur > --- > drivers/input/misc/bma150.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c > index 2124390..1fa8537 100644 > --- a/drivers/input/misc/bma150.c > +++ b/drivers/input/misc/bma150.c > @@ -207,7 +207,7 @@ static int bma150_set_mode(struct bma150_data *bma150, u8 > mode) > return error; > > if (mode == BMA150_MODE_NORMAL) > - msleep(2); > + usleep_range(2000, 2100); > > bma150->mode = mode; > return 0; > @@ -222,7 +222,7 @@ static int bma150_soft_reset(struct bma150_data *bma150) > if (error) > return error; > > - msleep(2); > + usleep_range(2000, 2100); > return 0; > } > > -- > 2.6.2 >
RE: Re: [PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs
Hello Mr. Vojtech Pavlik,
On 28 Nov 2016 17:23, "vojt...@ucw.cz" wrote:
>
> Hi.
>
> ADI_INIT_DELAY/ADI_DATA_DELAY doesn't have to be exact, and a longer
> sleep doesn't matter. In the initilization sequence - first chunk of
> your patch - a way too long delay could in theory make the device fail
> to initialize. What's critical is that the mdelay() calls are precise.
>
> One day I'll open my box of old joystick and re-test these drivers to
> see if they survived the years of kernel infrastructure updates ...
>
> Vojtech
>
Well, it seems to me that there is some kind of confusion, so I'd like to
clarify things about this patch.
As you have mentioned that in the initialization sequence, long delay could
in theory make the device fail to initialize - This patch actually solves
this problem.
msleep is built on jiffies / legacy timers and usleep_range is built on top
of hrtimers so the wakeup will be precise.
Source - https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
For example in initialization sequence, if we use msleep(4), then the process
could sleep for much more than 4 ms, say 6 ms or 10 ms or more depending on
machine architecture. Like on a machine with tick rate / HZ is defined to be
100 so msleep(4) will make the process to sleep for minimum 10 ms.
Whereas usleep_range(4000, 4100) will make sure that the process do not sleep
for more than 4100 us or 4.1 ms. So usleep_range is precise but msleep is not.
Originally, I added you in this patch to request you if you could help to
test this patch or provide contact points of individuals who could help
to test this patch as we do not have the hardware available with us?
Like this driver, we also need to test other joystick drivers as well like
gf2k.c, analog.c, sidewinder.c and gameport driver ns558.c as all have
similar problem.
Original patch link - https://patchwork.kernel.org/patch/9446201/
As we do not have hardware available, so we decided to split patch into
individual drivers and request to person who could support to test this patch
I have also applied the same patch in our driver whose hardware is available
with me and I found that wake up time became precise indeed and so I
decided to apply the same fix in other input subsystem drivers as well.
Thank you!
BR,
Aniroop Mathur
> On Mon, Nov 28, 2016 at 11:43:56AM +, Aniroop Mathur wrote:
> > msleep(1~20) may not do what the caller intends, and will often sleep
> > longer.
> > (~20 ms actual sleep for any value given in the 1~20ms range)
> > This is not the desired behaviour for many cases like device resume time,
> > device suspend time, device enable time, data reading time, etc.
> > Thus, change msleep to usleep_range for precise wakeups.
> >
> > Signed-off-by: Aniroop Mathur
> > ---
> > joystick/adi.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/joystick/adi.c b/joystick/adi.c
> > index d09cefa..6799bd9 100644
> > --- a/joystick/adi.c
> > +++ b/joystick/adi.c
> > @@ -47,8 +47,8 @@ MODULE_LICENSE("GPL");
> >
> > #define ADI_MAX_START200 /* Trigger to packet timeout
> > [200us] */
> > #define ADI_MAX_STROBE 40 /* Single bit timeout [40us]
> > */
> > -#define ADI_INIT_DELAY 10 /* Delay after init packet
> > [10ms] */
> > -#define ADI_DATA_DELAY 4 /* Delay after data packet
> > [4ms] */
> > +#define ADI_INIT_DELAY 1 /* Delay after init packet
> > [10ms] */
> > +#define ADI_DATA_DELAY 4000/* Delay after data packet
> > [4000us] */
> >
> > #define ADI_MAX_LENGTH 256
> > #define ADI_MIN_LENGTH 8
> > @@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport)
> > for (i = 0; seq[i]; i++) {
> > gameport_trigger(gameport);
> > if (seq[i] > 0)
> > - msleep(seq[i]);
> > + usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100);
> > if (seq[i] < 0) {
> > mdelay(-seq[i]);
> > udelay(-seq[i]*14); /* It looks like mdelay() is
> > off by approx 1.4% */
> > @@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport,
> > struct gameport_driver *drv)
> > gameport_set_poll_handler(gameport, adi_poll);
> > gameport_set_poll_interval(gameport, 20);
> >
> > - msleep(ADI_INIT_DELAY);
> > + usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100);
> > if (adi_read(port)) {
> > - msleep(ADI_DATA_DELAY);
> > + usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100);
> > adi_read(port);
> > }
> >
> > --
> > 2.6.4.windows.1
>
>
> --
> Vojtech Pavlik
[PATCH] Input: joystick: adi - change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, data reading time, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
joystick/adi.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/joystick/adi.c b/joystick/adi.c
index d09cefa..6799bd9 100644
--- a/joystick/adi.c
+++ b/joystick/adi.c
@@ -47,8 +47,8 @@ MODULE_LICENSE("GPL");
#define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */
#define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */
-#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */
-#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */
+#define ADI_INIT_DELAY 1 /* Delay after init packet [10ms] */
+#define ADI_DATA_DELAY 4000/* Delay after data packet [4000us] */
#define ADI_MAX_LENGTH 256
#define ADI_MIN_LENGTH 8
@@ -319,7 +319,7 @@ static void adi_init_digital(struct gameport *gameport)
for (i = 0; seq[i]; i++) {
gameport_trigger(gameport);
if (seq[i] > 0)
- msleep(seq[i]);
+ usleep_range(seq[i] * 1000, (seq[i] * 1000) + 100);
if (seq[i] < 0) {
mdelay(-seq[i]);
udelay(-seq[i]*14); /* It looks like mdelay() is
off by approx 1.4% */
@@ -512,9 +512,9 @@ static int adi_connect(struct gameport *gameport, struct
gameport_driver *drv)
gameport_set_poll_handler(gameport, adi_poll);
gameport_set_poll_interval(gameport, 20);
- msleep(ADI_INIT_DELAY);
+ usleep_range(ADI_INIT_DELAY, ADI_INIT_DELAY + 100);
if (adi_read(port)) {
- msleep(ADI_DATA_DELAY);
+ usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100);
adi_read(port);
}
--
2.6.4.windows.1
[PATCH] IIO: Change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, data reading time, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/iio/adc/exynos_adc.c | 2 +-
drivers/iio/pressure/bmp280-core.c | 14 +++---
drivers/staging/iio/meter/ade7753.c | 2 +-
drivers/staging/iio/meter/ade7753.h | 2 +-
drivers/staging/iio/meter/ade7754.c | 2 +-
drivers/staging/iio/meter/ade7754.h | 2 +-
drivers/staging/iio/meter/ade7758.h | 2 +-
drivers/staging/iio/meter/ade7758_core.c | 2 +-
drivers/staging/iio/meter/ade7759.c | 2 +-
drivers/staging/iio/meter/ade7759.h | 2 +-
drivers/staging/iio/meter/ade7854.c | 2 +-
drivers/staging/iio/meter/ade7854.h | 2 +-
12 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index c15756d..ad1775b 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -632,7 +632,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
input_report_key(info->input, BTN_TOUCH, 1);
input_sync(info->input);
- msleep(1);
+ usleep_range(1000, 1100);
};
writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
diff --git a/drivers/iio/pressure/bmp280-core.c
b/drivers/iio/pressure/bmp280-core.c
index e5a533c..4d18826 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -65,7 +65,7 @@ struct bmp280_data {
struct bmp180_calib calib;
struct regulator *vddd;
struct regulator *vdda;
- unsigned int start_up_time; /* in milliseconds */
+ unsigned int start_up_time; /* in microseconds */
/* log of base 2 of oversampling rate */
u8 oversampling_press;
@@ -935,14 +935,14 @@ int bmp280_common_probe(struct device *dev,
data->chip_info = &bmp180_chip_info;
data->oversampling_press = ilog2(8);
data->oversampling_temp = ilog2(1);
- data->start_up_time = 10;
+ data->start_up_time = 1;
break;
case BMP280_CHIP_ID:
indio_dev->num_channels = 2;
data->chip_info = &bmp280_chip_info;
data->oversampling_press = ilog2(16);
data->oversampling_temp = ilog2(2);
- data->start_up_time = 2;
+ data->start_up_time = 2000;
break;
case BME280_CHIP_ID:
indio_dev->num_channels = 3;
@@ -950,7 +950,7 @@ int bmp280_common_probe(struct device *dev,
data->oversampling_press = ilog2(16);
data->oversampling_humid = ilog2(16);
data->oversampling_temp = ilog2(2);
- data->start_up_time = 2;
+ data->start_up_time = 2000;
break;
default:
return -EINVAL;
@@ -979,7 +979,7 @@ int bmp280_common_probe(struct device *dev,
goto out_disable_vddd;
}
/* Wait to make sure we started up properly */
- mdelay(data->start_up_time);
+ usleep_range(data->start_up_time, data->start_up_time + 100);
/* Bring chip out of reset if there is an assigned GPIO line */
gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
@@ -1038,7 +1038,7 @@ int bmp280_common_probe(struct device *dev,
* Set autosuspend to two orders of magnitude larger than the
* start-up time.
*/
- pm_runtime_set_autosuspend_delay(dev, data->start_up_time *100);
+ pm_runtime_set_autosuspend_delay(dev, data->start_up_time / 10);
pm_runtime_use_autosuspend(dev);
pm_runtime_put(dev);
@@ -1101,7 +1101,7 @@ static int bmp280_runtime_resume(struct device *dev)
ret = regulator_enable(data->vdda);
if (ret)
return ret;
- msleep(data->start_up_time);
+ usleep_range(data->start_up_time, data->start_up_time + 100);
return data->chip_info->chip_config(data);
}
#endif /* CONFIG_PM */
diff --git a/drivers/staging/iio/meter/ade7753.c
b/drivers/staging/iio/meter/ade7753.c
index 4b5f05f..671dc99 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -377,7 +377,7 @@ static int ade7753_initial_setup(struct iio_dev *indio_dev)
}
ade7753_reset(dev);
- msleep(ADE7753_STARTUP_DELAY);
+ usleep_range(ADE7753_STARTUP_DELAY, ADE7753_STARTUP_DELAY + 100);
err_ret:
return ret;
diff --git a/drivers/staging/iio/meter/ade7753.h
b/drivers/staging/iio/me
[PATCH] Input: evdev: fix queueing of SYN_DROPPED event for EVIOCG[type] IOCTL case
Currently, when EVIOCG[type] ioctl call is issued and bits_to_user fails,
then SYN_DROPPED event is inserted in the event queue always.
However, it is not compulsory that some events are flushed out on every
EVIOCG[type] ioctl call like in case of empty event queue and in case when
EVIOCG[type] ioctl is issued for say A type of events but event queue does
not have any A type of events but some other type of events.
Therefore, insert SYN_DROPPED event only when some events have been flushed
out from event queue plus bits_to_user fails.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..f8b295e 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -108,9 +108,11 @@ static bool __evdev_is_filtered(struct evdev_client
*client,
}
/* flush queued events of type @type, caller must hold client->buffer_lock */
-static void __evdev_flush_queue(struct evdev_client *client, unsigned int type)
+static unsigned int __evdev_flush_queue(struct evdev_client *client,
+ unsigned int type)
{
unsigned int i, head, num;
+ unsigned int drop_count = 0;
unsigned int mask = client->bufsize - 1;
bool is_report;
struct input_event *ev;
@@ -129,9 +131,11 @@ static void __evdev_flush_queue(struct evdev_client
*client, unsigned int type)
if (ev->type == type) {
/* drop matched entry */
+ drop_count++;
continue;
} else if (is_report && !num) {
/* drop empty SYN_REPORT groups */
+ drop_count++;
continue;
} else if (head != i) {
/* move entry to fill the gap */
@@ -151,6 +155,7 @@ static void __evdev_flush_queue(struct evdev_client
*client, unsigned int type)
}
client->head = head;
+ return drop_count;
}
static void __evdev_queue_syn_dropped(struct evdev_client *client)
@@ -920,6 +925,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
int ret;
unsigned long *mem;
size_t len;
+ unsigned int drop_count = 0;
len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
mem = kmalloc(len, GFP_KERNEL);
@@ -933,12 +939,12 @@ static int evdev_handle_get_val(struct evdev_client
*client,
spin_unlock(&dev->event_lock);
- __evdev_flush_queue(client, type);
+ drop_count = __evdev_flush_queue(client, type);
spin_unlock_irq(&client->buffer_lock);
ret = bits_to_user(mem, maxbit, maxlen, p, compat);
- if (ret < 0)
+ if (ret < 0 && drop_count > 0)
evdev_queue_syn_dropped(client);
kfree(mem);
--
2.6.2
[PATCH] Input: Change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, data reading time, etc.
Thus, change msleep to usleep_range for precise wakeups.
Signed-off-by: Aniroop Mathur
---
drivers/input/gameport/ns558.c | 4 ++--
drivers/input/joystick/adi.c| 4 ++--
drivers/input/joystick/analog.c | 12 ++--
drivers/input/joystick/gf2k.c | 8
drivers/input/joystick/sidewinder.c | 24
drivers/input/keyboard/lm8323.c | 2 +-
drivers/input/mouse/navpoint.c | 2 +-
drivers/input/mouse/pxa930_trkball.c| 2 +-
drivers/input/mouse/synaptics_i2c.c | 6 +++---
drivers/input/touchscreen/ads7846.c | 4 ++--
drivers/input/touchscreen/edt-ft5x06.c | 4 ++--
drivers/input/touchscreen/w90p910_ts.c | 4 ++--
drivers/input/touchscreen/zylonite-wm97xx.c | 4 ++--
13 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/input/gameport/ns558.c b/drivers/input/gameport/ns558.c
index 7c21784..046cb28 100644
--- a/drivers/input/gameport/ns558.c
+++ b/drivers/input/gameport/ns558.c
@@ -98,7 +98,7 @@ static int ns558_isa_probe(int io)
release_region(io, 1);
return -ENODEV;
}
- msleep(3);
+ usleep_range(3000, 3100);
/*
* After some time (4ms) the axes shouldn't change anymore.
*/
@@ -124,7 +124,7 @@ static int ns558_isa_probe(int io)
outb(0xff, io & (-1 << i));
for (j = b = 0; j < 1000; j++)
if (inb(io & (-1 << i)) != inb((io & (-1 << i)) + (1 <<
i) - 1)) b++;
- msleep(3);
+ usleep_range(3000, 3100);
if (b > 300) { /* We allow 30%
difference */
release_region(io & (-1 << i), (1 << i));
diff --git a/drivers/input/joystick/adi.c b/drivers/input/joystick/adi.c
index d09cefa..f1955bf 100644
--- a/drivers/input/joystick/adi.c
+++ b/drivers/input/joystick/adi.c
@@ -48,7 +48,7 @@ MODULE_LICENSE("GPL");
#define ADI_MAX_START 200 /* Trigger to packet timeout [200us] */
#define ADI_MAX_STROBE 40 /* Single bit timeout [40us] */
#define ADI_INIT_DELAY 10 /* Delay after init packet [10ms] */
-#define ADI_DATA_DELAY 4 /* Delay after data packet [4ms] */
+#define ADI_DATA_DELAY 4000/* Delay after data packet [4000us] */
#define ADI_MAX_LENGTH 256
#define ADI_MIN_LENGTH 8
@@ -514,7 +514,7 @@ static int adi_connect(struct gameport *gameport, struct
gameport_driver *drv)
msleep(ADI_INIT_DELAY);
if (adi_read(port)) {
- msleep(ADI_DATA_DELAY);
+ usleep_range(ADI_DATA_DELAY, ADI_DATA_DELAY + 100);
adi_read(port);
}
diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
index 3d8ff09..2891704 100644
--- a/drivers/input/joystick/analog.c
+++ b/drivers/input/joystick/analog.c
@@ -88,7 +88,7 @@ MODULE_PARM_DESC(map, "Describes analog joysticks
type/capabilities");
#define ANALOG_EXTENSIONS 0x7ff00
#define ANALOG_GAMEPAD 0x8
-#define ANALOG_MAX_TIME3 /* 3 ms */
+#define ANALOG_MAX_TIME3000/* 3000 us */
#define ANALOG_LOOP_TIME 2000/* 2 * loop */
#define ANALOG_SAITEK_DELAY200 /* 200 us */
#define ANALOG_SAITEK_TIME 2000/* 2000 us */
@@ -257,7 +257,7 @@ static int analog_cooked_read(struct analog_port *port)
int i, j;
loopout = (ANALOG_LOOP_TIME * port->loop) / 1000;
- timeout = ANALOG_MAX_TIME * port->speed;
+ timeout = (ANALOG_MAX_TIME / 1000) * port->speed;
local_irq_save(flags);
gameport_trigger(gameport);
@@ -625,20 +625,20 @@ static int analog_init_port(struct gameport *gameport,
struct gameport_driver *d
gameport_trigger(gameport);
t = gameport_read(gameport);
- msleep(ANALOG_MAX_TIME);
+ usleep_range(ANALOG_MAX_TIME, ANALOG_MAX_TIME + 100);
port->mask = (gameport_read(gameport) ^ t) & t & 0xf;
port->fuzz = (port->speed * ANALOG_FUZZ_MAGIC) / port->loop /
1000 + ANALOG_FUZZ_BITS;
for (i = 0; i < ANALOG_INIT_RETRIES; i++) {
if (!analog_cooked_read(port))
break;
- msleep(ANALOG_MAX_TIME);
+ usleep_range(ANALOG_MAX_TIME, ANALOG_MAX_TIME + 100);
}
u = v = 0;
- msleep(ANALOG_MAX_TIME)
[PATCH] Input: Change msleep to usleep_range for small msecs
msleep(1~20) may not do what the caller intends, and will often sleep longer. (~20 ms actual sleep for any value given in the 1~20ms range) This is not the desired behaviour for many cases like device resume time, device suspend time, device enable time, etc. Thus, change msleep to usleep_range for precise wakeups. Signed-off-by: Aniroop Mathur --- drivers/input/misc/bma150.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c index 2124390..1fa8537 100644 --- a/drivers/input/misc/bma150.c +++ b/drivers/input/misc/bma150.c @@ -207,7 +207,7 @@ static int bma150_set_mode(struct bma150_data *bma150, u8 mode) return error; if (mode == BMA150_MODE_NORMAL) - msleep(2); + usleep_range(2000, 2100); bma150->mode = mode; return 0; @@ -222,7 +222,7 @@ static int bma150_soft_reset(struct bma150_data *bma150) if (error) return error; - msleep(2); + usleep_range(2000, 2100); return 0; } -- 2.6.2
RE: Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Hello Mr. Torokhov,
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> {
>> struct input_event ev;
>> +struct input_event *last_ev;
>> ktime_t time;
>> +unsigned int mask = client->bufsize - 1;
>> +
>> +/* capture last event stored in the buffer */
>> +last_ev = &client->buffer[(client->head - 1) & mask];
> I am uneasy with this "looking back" into the queue. How can we be sure
> that we are looking at the right event? As far as I can tell we do not
> know if that event has been consumed or not.
Well, I think that for an exceptional case where even if last event is
consumed then also it is the right event to proceed with.
Before mentioning then exceptional case, there is a need to fix a bug in
calling evdev_queue_syn_dropped in evdev_handle_get_val function.
The bug is:
Currently we are calling evdev_queue_syn_dropped without taking care
whether some events have actually been flushed out or not by calling
__evdev_flush_queue function. But ideally we should call
evdev_queue_syn_dropped only when some events are being flushed out.
For example: If client requests for EV_KEY events and queue only has
EV_LED type of events, then it is possible that bits_to_user fails
but no events get flushed out from queue and hence we should not be
inserting SYN_DROPPED event for this case.
So to fix this,
I think we need to return the number of events dropped from function
__evdev_flush_queue, say n is that value. So only if n > 0, then only
evdev_queue_syn_dropped should be called.
- __evdev_flush_queue(client, type);
+ n = __evdev_flush_queue(client, type);
- if (ret < 0)
+ if (ret < 0 && n > 0)
evdev_queue_syn_dropped(client);
Along with this bug fix, it will also handle the case when queue is
empty at time of invoking EVIOCG[type] ioctl call so after including
this fix, we can remove explicit handling of queue empty case from this patch.
After having this change, that exceptional case where even if the last event
is consumed, it is still the right event is:
Suppose client request from EV_KEY type of events using EVIOCG[type] ioctl call
and queue does contain EV_KEY type of events and bits_to_user fails too.
Now, after this when we invoke evdev_queue_syn_dropped function, there is a
chance that queue get fully consumed i.e. head == tail. So last event is
consumed as well. But since we need to send SYN_DROPPED event for this case
to indicate to user space that some events have been dropped, so we also have
to check whether we need to insert a SYN_REPORT event or not right after
SYN_DROPPED event using that last consumed event. And I think that it is for
sure that this last event is actually SYN_REPORT event since input core
always send events in packets so SYN_REPORT is always the last event forwarded
by input core to evdev.
Does the above mentioned points seem okay to you?
--
Best Regards,
Aniroop Mathur
- Original Message -
Sender : Dmitry Torokhov
Date : 2016-11-20 00:30 (GMT+5:30)
Title : Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after
syn_dropped event
Hi Anoroop,
On Wed, Oct 05, 2016 at 12:42:56AM +0530, Aniroop Mathur wrote:
> If last event dropped in the old queue was EV_SYN/SYN_REPORT, then lets
> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
> so that clients would not ignore next valid full packet events.
>
> Signed-off-by: Aniroop Mathur
>
> Difference from v8:
> Added check for handling EVIOCG[type] ioctl for queue empty case
> ---
> drivers/input/evdev.c | 52
>++-
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..69407ff 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>*client, unsigned int type)
> static void __evdev_queue_syn_dropped(struct evdev_client *client)
> {
> struct input_event ev;
> +struct input_event *last_ev;
> ktime_t time;
> +unsigned int mask = client->bufsize - 1;
> +
> +/* capture last event stored in the buffer */
> +last_ev = &client->buffer[(client->head - 1) & mask];
I am uneasy with this "looking back" into the queue. How can we be sure
that we are looking at the right event? As far as I can tell we do not
know if that event has been consumed or not.
>
> time = client->clk_type == EV_CLK_REAL ?
> ktime_get_real() :
> @@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct
>evdev_client *client)
> ev.value = 0;
>
>
Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Hello Mr. Torokhov,
As you can see that this patch is pending for a long time,
I request you to please review it at the earliest possible.
Thanks,
Aniroop Mathur
On Thu, Oct 13, 2016 at 12:05 AM, Aniroop Mathur
wrote:
> Hello Mr. Torokhov,
>
> Hope you are doing great!
>
> Could you please help to update about below version of the patch?
>
> --
> Copying text about last two problems in v8:
>
> Problem 1: Handle EVIOCG[type] for queue empty case
> --> Done
> Queue empty condition needs to be added before calling
> evdev_queue_syn_dropped.
> Since there is a rare chance that new packet gets inserted between buffer_lock
> mutex unlocking and copying events to user space, so I will check whether
> queue
> is empty or not before __evdev_flush_queue and then use it before invoking
> evdev_queue_syn_dropped.
> And if queue is not empty then there is no problem as after flushing some
> events we always have atleast one SYN_REPORT event left in queue.
>
> Problem 2: We try to pass full packets to clients, but there is no guarantee.
> --> Done
> From my earlier patch discussion regarding dropping syn_report in between a
> single packet it was deduced that since no driver is currently using that code
> so there is no impact. Hence for this problem too, we are good to go.~
> (Although from code reading/learning perspective, I would still suggest to not
> have code of forceful sun_report event addition even if no driver using that
> code :-) )
>
> Have a good day!
>
> Best Regards,
> Aniroop Mathur
>
>
>> On Wed, Oct 5, 2016 at 12:42 AM, Aniroop Mathur wrote:
>>> If last event dropped in the old queue was EV_SYN/SYN_REPORT, then lets
>>> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>>> so that clients would not ignore next valid full packet events.
>>>
>>> Signed-off-by: Aniroop Mathur
>>>
>>> Difference from v8:
>>> Added check for handling EVIOCG[type] ioctl for queue empty case
>>> ---
>>> drivers/input/evdev.c | 52
>>> ++-
>>> 1 file changed, 39 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>>> index e9ae3d5..69407ff 100644
>>> --- a/drivers/input/evdev.c
>>> +++ b/drivers/input/evdev.c
>>> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>>> *client, unsigned int type)
>>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>> {
>>> struct input_event ev;
>>> + struct input_event *last_ev;
>>> ktime_t time;
>>> + unsigned int mask = client->bufsize - 1;
>>> +
>>> + /* capture last event stored in the buffer */
>>> + last_ev = &client->buffer[(client->head - 1) & mask];
>>>
>>> time = client->clk_type == EV_CLK_REAL ?
>>> ktime_get_real() :
>>> @@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct
>>> evdev_client *client)
>>> ev.value = 0;
>>>
>>> client->buffer[client->head++] = ev;
>>> - client->head &= client->bufsize - 1;
>>> + client->head &= mask;
>>>
>>> if (unlikely(client->head == client->tail)) {
>>> /* drop queue but keep our SYN_DROPPED event */
>>> - client->tail = (client->head - 1) & (client->bufsize - 1);
>>> + client->tail = (client->head - 1) & mask;
>>> client->packet_head = client->tail;
>>> }
>>> +
>>> + /*
>>> +* If last packet was completely stored, then queue SYN_REPORT
>>> +* so that clients would not ignore next valid full packet
>>> +*/
>>> + if (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
>>> + last_ev->time = ev.time;
>>> + client->buffer[client->head++] = *last_ev;
>>> + client->head &= mask;
>>> + client->packet_head = client->head;
>>> +
>>> + /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
>>> + if (unlikely(client->head == client->tail))
>>> + client->tail = (client->head - 2) & mask;
>>> + }
>>> }
>>>
>>> static void evde
Re: [PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Hello Mr. Torokhov,
Hope you are doing great!
Could you please help to update about below version of the patch?
--
Copying text about last two problems in v8:
Problem 1: Handle EVIOCG[type] for queue empty case
--> Done
Queue empty condition needs to be added before calling evdev_queue_syn_dropped.
Since there is a rare chance that new packet gets inserted between buffer_lock
mutex unlocking and copying events to user space, so I will check whether queue
is empty or not before __evdev_flush_queue and then use it before invoking
evdev_queue_syn_dropped.
And if queue is not empty then there is no problem as after flushing some
events we always have atleast one SYN_REPORT event left in queue.
Problem 2: We try to pass full packets to clients, but there is no guarantee.
--> Done
>From my earlier patch discussion regarding dropping syn_report in between a
single packet it was deduced that since no driver is currently using that code
so there is no impact. Hence for this problem too, we are good to go.~
(Although from code reading/learning perspective, I would still suggest to not
have code of forceful sun_report event addition even if no driver using that
code :-) )
Have a good day!
Best Regards,
Aniroop Mathur
> On Wed, Oct 5, 2016 at 12:42 AM, Aniroop Mathur wrote:
>> If last event dropped in the old queue was EV_SYN/SYN_REPORT, then lets
>> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>> so that clients would not ignore next valid full packet events.
>>
>> Signed-off-by: Aniroop Mathur
>>
>> Difference from v8:
>> Added check for handling EVIOCG[type] ioctl for queue empty case
>> ---
>> drivers/input/evdev.c | 52
>> ++-
>> 1 file changed, 39 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..69407ff 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> {
>> struct input_event ev;
>> + struct input_event *last_ev;
>> ktime_t time;
>> + unsigned int mask = client->bufsize - 1;
>> +
>> + /* capture last event stored in the buffer */
>> + last_ev = &client->buffer[(client->head - 1) & mask];
>>
>> time = client->clk_type == EV_CLK_REAL ?
>> ktime_get_real() :
>> @@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct
>> evdev_client *client)
>> ev.value = 0;
>>
>> client->buffer[client->head++] = ev;
>> - client->head &= client->bufsize - 1;
>> + client->head &= mask;
>>
>> if (unlikely(client->head == client->tail)) {
>> /* drop queue but keep our SYN_DROPPED event */
>> - client->tail = (client->head - 1) & (client->bufsize - 1);
>> + client->tail = (client->head - 1) & mask;
>> client->packet_head = client->tail;
>> }
>> +
>> + /*
>> +* If last packet was completely stored, then queue SYN_REPORT
>> +* so that clients would not ignore next valid full packet
>> +*/
>> + if (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
>> + last_ev->time = ev.time;
>> + client->buffer[client->head++] = *last_ev;
>> + client->head &= mask;
>> + client->packet_head = client->head;
>> +
>> + /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
>> + if (unlikely(client->head == client->tail))
>> + client->tail = (client->head - 2) & mask;
>> + }
>> }
>>
>> static void evdev_queue_syn_dropped(struct evdev_client *client)
>> @@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client
>> *client, unsigned int clkid)
>> spin_lock_irqsave(&client->buffer_lock, flags);
>>
>> if (client->head != client->tail) {
>> - client->packet_head = client->head = client->tail;
>> + client->packet_head = client->tail = client->head;
>> __evdev_queue_syn_dropped(client);
>> }
>>
>> @@ -231,22 +251,24 @@ static int e
[PATCH] [v9]Input: evdev: fix bug of dropping valid packet after syn_dropped event
If last event dropped in the old queue was EV_SYN/SYN_REPORT, then lets
generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
so that clients would not ignore next valid full packet events.
Signed-off-by: Aniroop Mathur
Difference from v8:
Added check for handling EVIOCG[type] ioctl for queue empty case
---
drivers/input/evdev.c | 52 ++-
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..69407ff 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
*client, unsigned int type)
static void __evdev_queue_syn_dropped(struct evdev_client *client)
{
struct input_event ev;
+ struct input_event *last_ev;
ktime_t time;
+ unsigned int mask = client->bufsize - 1;
+
+ /* capture last event stored in the buffer */
+ last_ev = &client->buffer[(client->head - 1) & mask];
time = client->clk_type == EV_CLK_REAL ?
ktime_get_real() :
@@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct evdev_client
*client)
ev.value = 0;
client->buffer[client->head++] = ev;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/* drop queue but keep our SYN_DROPPED event */
- client->tail = (client->head - 1) & (client->bufsize - 1);
+ client->tail = (client->head - 1) & mask;
client->packet_head = client->tail;
}
+
+ /*
+* If last packet was completely stored, then queue SYN_REPORT
+* so that clients would not ignore next valid full packet
+*/
+ if (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
+ last_ev->time = ev.time;
+ client->buffer[client->head++] = *last_ev;
+ client->head &= mask;
+ client->packet_head = client->head;
+
+ /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
+ if (unlikely(client->head == client->tail))
+ client->tail = (client->head - 2) & mask;
+ }
}
static void evdev_queue_syn_dropped(struct evdev_client *client)
@@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
spin_lock_irqsave(&client->buffer_lock, flags);
if (client->head != client->tail) {
- client->packet_head = client->head = client->tail;
+ client->packet_head = client->tail = client->head;
__evdev_queue_syn_dropped(client);
}
@@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client
*client, unsigned int clkid)
static void __pass_event(struct evdev_client *client,
const struct input_event *event)
{
+ unsigned int mask = client->bufsize - 1;
+
client->buffer[client->head++] = *event;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/*
* This effectively "drops" all unconsumed events, leaving
-* EV_SYN/SYN_DROPPED plus the newest event in the queue.
+* EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
+* newest event in the queue.
*/
- client->tail = (client->head - 2) & (client->bufsize - 1);
-
- client->buffer[client->tail].time = event->time;
- client->buffer[client->tail].type = EV_SYN;
- client->buffer[client->tail].code = SYN_DROPPED;
- client->buffer[client->tail].value = 0;
+ client->head = (client->head - 1) & mask;
+ client->packet_head = client->tail = client->head;
+ __evdev_queue_syn_dropped(client);
- client->packet_head = client->tail;
+ client->buffer[client->head++] = *event;
+ client->head &= mask;
+ /* No need to check for buffer overflow as it just occurred */
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
@@ -920,6 +942,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
int ret;
unsigned long *mem;
size_t len;
+ bool is_queue_empty;
len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
mem = kmalloc(len, GFP_KERNEL);
@@ -933,12 +956,
Re: [PATCH] [v8]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Dear Mr. Torokhov,
Greetings of the day!!
Somehow, it has been a while since this patch is further discussed.
I am top posting as to finish off the remaining two points.
>From the earlier conversations, I could deduce that this patch needs to be
further checked for 2 problems as follows:
Problem 1: Handle EVIOCG[type] for queue empty case
--> For this, I will send you the the new patch version in a while.
Queue empty condition needs to be added before calling evdev_queue_syn_dropped.
Since there is a rare chance that new packet gets inserted between buffer_lock
mutex unlocking and copying events to user space, so I will check whether queue
is empty or not before __evdev_flush_queue and then use it before invoking
evdev_queue_syn_dropped.
And if queue is not empty then there is no problem as after flushing some
events we always have atleast one SYN_REPORT event left in queue.
Problem 2: We try to pass full packets to clients, but there is no guarantee.
--> From my earlier patch discussion regarding dropping syn_report in between a
single packet it was deduced that since no driver is currently using that code
so there is no impact. Hence for this problem too, we are good to go.
Do you have any more comments?
Best Regards,
Aniroop Mathur
On Sun, Jan 24, 2016 at 1:35 AM, Aniroop Mathur
wrote:
> On Sun, Jan 24, 2016 at 12:09 AM, Dmitry Torokhov
> wrote:
>> On Sat, Jan 23, 2016 at 11:29:29PM +0530, Aniroop Mathur wrote:
>>> Hi Mr. Torokhov,
>>>
>>> On Fri, Jan 22, 2016 at 12:47 AM, Dmitry Torokhov
>>> wrote:
>>> > Hi Anoroop,
>>> >
>>> > On Thu, Jan 21, 2016 at 11:07:19PM +0530, Aniroop Mathur wrote:
>>> >> If last event dropped in the old queue was EVi_SYN/SYN_REPORT, then lets
>>> >> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>>> >> so that clients would not ignore next valid full packet events.
>>> >>
>>> >> Signed-off-by: Aniroop Mathur
>>> >> ---
>>> >> drivers/input/evdev.c | 46
>>> >> ++
>>> >> 1 file changed, 34 insertions(+), 12 deletions(-)
>>> >>
>>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>>> >> index e9ae3d5..821b68a 100644
>>> >> --- a/drivers/input/evdev.c
>>> >> +++ b/drivers/input/evdev.c
>>> >> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>>> >> *client, unsigned int type)
>>> >> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>> >> {
>>> >> struct input_event ev;
>>> >> + struct input_event *last_ev;
>>> >> ktime_t time;
>>> >> + unsigned int mask = client->bufsize - 1;
>>> >> +
>>> >> + /* capture last event stored in the buffer */
>>> >> + last_ev = &client->buffer[(client->head - 1) & mask];
>>> >
>>> > I have still the same comment. How do you know that event at last_ev
>>> > position is in fact valid and unconsumed yet event. Also, you need to
>>> > figure out not only if queue contains last SYN event, but also to handle
>>> > the case where the queue is empty and client has consumed either full or
>>> > partial packet at the time you are queueing the drop.
>>> >
>>>
>>> Could you please explain what you mean exactly so that I could answer it
>>> properly?
>>>
>>> From what I understood, it seems to me that there is no problem related to
>>> validity, unconsumption, empty queue, full/partial packet.
>>> I would like to explain for clock change request case so that you can know
>>> my understanding for your question.
>>>
>>> Clock change request case:
>>>
>>> 1.1 About last event being valid and unconsumed:
>>> We flush buffer and queue syn_dropped only when buffer is not empty. So
>>> there
>>> will be always be atleast one event in buffer that is not consumed and is
>>> ofcourse valid.
>>>
>>> 1.2 About queue is empty
>>> If not empty, we do not flush or add syn_dropped at all.
>>
>> Clock type change is not the only time we queue SYN_DROP, the other time
>> is when we fail to handle EVIOCG[type] (during which we remove some
>> events from the queue). Queue may be empty when these ioctls are issued.
>>
>
> yeah, ideally it should be changed to:
> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> if (ret < 0)
> + if (cl
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
On Thu, Apr 7, 2016 at 2:13 AM, Aniroop Mathur wrote: > Hello Mr. Henrik, > > On Thu, Apr 7, 2016 at 1:21 AM, Henrik Rydberg 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
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
Hello Mr. Henrik, On Thu, Apr 7, 2016 at 1:21 AM, Henrik Rydberg 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
On Wed, Apr 6, 2016 at 11:08 PM, Dmitry Torokhov
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
>> wrote:
>> > Hello Mr. Torokhov,
>> >
>> > First of all, Thank you for your reply.
>> >
>> > On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
>> > 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
>> >>> 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, rig
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
On Sat, Apr 2, 2016 at 10:31 PM, Aniroop Mathur
wrote:
> Hello Mr. Torokhov,
>
> First of all, Thank you for your reply.
>
> On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
> 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 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
[PATCH] Input: Do not add SYN_REPORT in between a single packet data
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
---
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
Hello Mr. Torokhov,
First of all, Thank you for your reply.
On Sat, Apr 2, 2016 at 3:21 AM, Dmitry Torokhov
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 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
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
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
wrote:
> Hello Mr. Torokhov / Mr. Henry,
>
> On Wed, Mar 16, 2016 at 11:54 PM, Aniroop Mathur
> 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
>> wrote:
>>> Hi Henrik,
>>>
>>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg 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
Hello Mr. Torokhov / Mr. Henry,
On Wed, Mar 16, 2016 at 11:54 PM, Aniroop Mathur
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
> wrote:
>> Hi Henrik,
>>
>> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg 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
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
wrote:
> Hi Henrik,
>
> On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg 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
Hi Henrik,
On Thu, Mar 10, 2016 at 7:15 PM, Henrik Rydberg 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
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 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
> ---
> 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
>
[PATCH] Input: Do not add SYN_REPORT in between a single packet data
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
---
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] [v8]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Hello Mr. Torokhov,
On 1/25/16, Aniroop Mathur wrote:
> On Sun, Jan 24, 2016 at 1:35 AM, Aniroop Mathur
> wrote:
>> On Sun, Jan 24, 2016 at 12:09 AM, Dmitry Torokhov
>> wrote:
>>> On Sat, Jan 23, 2016 at 11:29:29PM +0530, Aniroop Mathur wrote:
>>>> Hi Mr. Torokhov,
>>>>
>>>> On Fri, Jan 22, 2016 at 12:47 AM, Dmitry Torokhov
>>>> wrote:
>>>> > Hi Anoroop,
>>>> >
>>>> > On Thu, Jan 21, 2016 at 11:07:19PM +0530, Aniroop Mathur wrote:
>>>> >> If last event dropped in the old queue was EVi_SYN/SYN_REPORT, then
>>>> >> lets
>>>> >> generate EV_SYN/SYN_REPORT immediately after queing
>>>> >> EV_SYN/SYN_DROPPED
>>>> >> so that clients would not ignore next valid full packet events.
>>>> >>
>>>> >> Signed-off-by: Aniroop Mathur
>>>> >> ---
>>>> >> drivers/input/evdev.c | 46
>>>> >> ++
>>>> >> 1 file changed, 34 insertions(+), 12 deletions(-)
>>>> >>
>>>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>>>> >> index e9ae3d5..821b68a 100644
>>>> >> --- a/drivers/input/evdev.c
>>>> >> +++ b/drivers/input/evdev.c
>>>> >> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct
>>>> >> evdev_client *client, unsigned int type)
>>>> >> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>>> >> {
>>>> >> struct input_event ev;
>>>> >> + struct input_event *last_ev;
>>>> >> ktime_t time;
>>>> >> + unsigned int mask = client->bufsize - 1;
>>>> >> +
>>>> >> + /* capture last event stored in the buffer */
>>>> >> + last_ev = &client->buffer[(client->head - 1) & mask];
>>>> >
>>>> > I have still the same comment. How do you know that event at last_ev
>>>> > position is in fact valid and unconsumed yet event. Also, you need to
>>>> > figure out not only if queue contains last SYN event, but also to
>>>> > handle
>>>> > the case where the queue is empty and client has consumed either full
>>>> > or
>>>> > partial packet at the time you are queueing the drop.
>>>> >
>>>>
>>>> Could you please explain what you mean exactly so that I could answer
>>>> it
>>>> properly?
>>>>
>>>> From what I understood, it seems to me that there is no problem related
>>>> to
>>>> validity, unconsumption, empty queue, full/partial packet.
>>>> I would like to explain for clock change request case so that you can
>>>> know
>>>> my understanding for your question.
>>>>
>>>> Clock change request case:
>>>>
>>>> 1.1 About last event being valid and unconsumed:
>>>> We flush buffer and queue syn_dropped only when buffer is not empty. So
>>>> there
>>>> will be always be atleast one event in buffer that is not consumed and
>>>> is
>>>> ofcourse valid.
>>>>
>>>> 1.2 About queue is empty
>>>> If not empty, we do not flush or add syn_dropped at all.
>>>
>>> Clock type change is not the only time we queue SYN_DROP, the other time
>>> is when we fail to handle EVIOCG[type] (during which we remove some
>>> events from the queue). Queue may be empty when these ioctls are issued.
>>>
>>
>> yeah, ideally it should be changed to:
>> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
>> if (ret < 0)
>> + if (client->head != client->tail)
>> - evdev_queue_syn_dropped(client);
>> + evdev_queue_syn_dropped(client);
>>
>> Firstly, there is a need to follow protocol of SYN_REPORT as mentioned in
>> next section.
>>
>
> and yes, there is need to make many more changes to have correct
> last_event for this case. But it seems really complex.
>
>> Unless syn_report does not denote end of a packet,
>> it is okay without this change too because last event stored would be
>> syn_report and after flushing, syn_report will still be at the end and
>> with empty queue too if syn_dropped is queued then we have added
>> syn_report
>> to not ignore upcoming valid packet.
Re: [PATCH] [v8]Input: evdev: fix bug of dropping valid packet after syn_dropped event
On Sun, Jan 24, 2016 at 1:35 AM, Aniroop Mathur
wrote:
> On Sun, Jan 24, 2016 at 12:09 AM, Dmitry Torokhov
> wrote:
>> On Sat, Jan 23, 2016 at 11:29:29PM +0530, Aniroop Mathur wrote:
>>> Hi Mr. Torokhov,
>>>
>>> On Fri, Jan 22, 2016 at 12:47 AM, Dmitry Torokhov
>>> wrote:
>>> > Hi Anoroop,
>>> >
>>> > On Thu, Jan 21, 2016 at 11:07:19PM +0530, Aniroop Mathur wrote:
>>> >> If last event dropped in the old queue was EVi_SYN/SYN_REPORT, then lets
>>> >> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>>> >> so that clients would not ignore next valid full packet events.
>>> >>
>>> >> Signed-off-by: Aniroop Mathur
>>> >> ---
>>> >> drivers/input/evdev.c | 46
>>> >> ++
>>> >> 1 file changed, 34 insertions(+), 12 deletions(-)
>>> >>
>>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>>> >> index e9ae3d5..821b68a 100644
>>> >> --- a/drivers/input/evdev.c
>>> >> +++ b/drivers/input/evdev.c
>>> >> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>>> >> *client, unsigned int type)
>>> >> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>>> >> {
>>> >> struct input_event ev;
>>> >> + struct input_event *last_ev;
>>> >> ktime_t time;
>>> >> + unsigned int mask = client->bufsize - 1;
>>> >> +
>>> >> + /* capture last event stored in the buffer */
>>> >> + last_ev = &client->buffer[(client->head - 1) & mask];
>>> >
>>> > I have still the same comment. How do you know that event at last_ev
>>> > position is in fact valid and unconsumed yet event. Also, you need to
>>> > figure out not only if queue contains last SYN event, but also to handle
>>> > the case where the queue is empty and client has consumed either full or
>>> > partial packet at the time you are queueing the drop.
>>> >
>>>
>>> Could you please explain what you mean exactly so that I could answer it
>>> properly?
>>>
>>> From what I understood, it seems to me that there is no problem related to
>>> validity, unconsumption, empty queue, full/partial packet.
>>> I would like to explain for clock change request case so that you can know
>>> my understanding for your question.
>>>
>>> Clock change request case:
>>>
>>> 1.1 About last event being valid and unconsumed:
>>> We flush buffer and queue syn_dropped only when buffer is not empty. So
>>> there
>>> will be always be atleast one event in buffer that is not consumed and is
>>> ofcourse valid.
>>>
>>> 1.2 About queue is empty
>>> If not empty, we do not flush or add syn_dropped at all.
>>
>> Clock type change is not the only time we queue SYN_DROP, the other time
>> is when we fail to handle EVIOCG[type] (during which we remove some
>> events from the queue). Queue may be empty when these ioctls are issued.
>>
>
> yeah, ideally it should be changed to:
> ret = bits_to_user(mem, maxbit, maxlen, p, compat);
> if (ret < 0)
> + if (client->head != client->tail)
> - evdev_queue_syn_dropped(client);
> + evdev_queue_syn_dropped(client);
>
> Firstly, there is a need to follow protocol of SYN_REPORT as mentioned in
> next section.
>
and yes, there is need to make many more changes to have correct
last_event for this case. But it seems really complex.
> Unless syn_report does not denote end of a packet,
> it is okay without this change too because last event stored would be
> syn_report and after flushing, syn_report will still be at the end and
> with empty queue too if syn_dropped is queued then we have added syn_report
> to not ignore upcoming valid packet.
>
>>>
>>> 1.3 About consumption of full or partial packet
>>> If client has consumed full packet, then buffer will look like,
>>> ... X Y S(consumed) ... X Y S
>>> As we always store packets keeping buffer lock and not single events, so
>>> there
>>> will always be syn_report in the end.
>>
>> We try to pass full packets to clients, but there is no guarantee. We
>> only estimate number of events in device's packet, not guarantee that it
>> is correct size.
>>
>
> As per d
[PATCH] Input: evdev: correct handling of memory allocation failure of evdev_client
Lets fix twice checking of memory allocation failure of evdev_client
structure when allocated successfully.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..102b5d9 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -503,10 +503,11 @@ static int evdev_open(struct inode *inode, struct file
*file)
int error;
client = kzalloc(size, GFP_KERNEL | __GFP_NOWARN);
- if (!client)
+ if (!client) {
client = vzalloc(size);
- if (!client)
- return -ENOMEM;
+ if (!client)
+ return -ENOMEM;
+ }
client->bufsize = bufsize;
spin_lock_init(&client->buffer_lock);
--
2.6.2
Re: [PATCH] [v8]Input: evdev: fix bug of dropping valid packet after syn_dropped event
On Sun, Jan 24, 2016 at 12:09 AM, Dmitry Torokhov
wrote:
> On Sat, Jan 23, 2016 at 11:29:29PM +0530, Aniroop Mathur wrote:
>> Hi Mr. Torokhov,
>>
>> On Fri, Jan 22, 2016 at 12:47 AM, Dmitry Torokhov
>> wrote:
>> > Hi Anoroop,
>> >
>> > On Thu, Jan 21, 2016 at 11:07:19PM +0530, Aniroop Mathur wrote:
>> >> If last event dropped in the old queue was EVi_SYN/SYN_REPORT, then lets
>> >> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>> >> so that clients would not ignore next valid full packet events.
>> >>
>> >> Signed-off-by: Aniroop Mathur
>> >> ---
>> >> drivers/input/evdev.c | 46 ++
>> >> 1 file changed, 34 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> >> index e9ae3d5..821b68a 100644
>> >> --- a/drivers/input/evdev.c
>> >> +++ b/drivers/input/evdev.c
>> >> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>> >> *client, unsigned int type)
>> >> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> >> {
>> >> struct input_event ev;
>> >> + struct input_event *last_ev;
>> >> ktime_t time;
>> >> + unsigned int mask = client->bufsize - 1;
>> >> +
>> >> + /* capture last event stored in the buffer */
>> >> + last_ev = &client->buffer[(client->head - 1) & mask];
>> >
>> > I have still the same comment. How do you know that event at last_ev
>> > position is in fact valid and unconsumed yet event. Also, you need to
>> > figure out not only if queue contains last SYN event, but also to handle
>> > the case where the queue is empty and client has consumed either full or
>> > partial packet at the time you are queueing the drop.
>> >
>>
>> Could you please explain what you mean exactly so that I could answer it
>> properly?
>>
>> From what I understood, it seems to me that there is no problem related to
>> validity, unconsumption, empty queue, full/partial packet.
>> I would like to explain for clock change request case so that you can know
>> my understanding for your question.
>>
>> Clock change request case:
>>
>> 1.1 About last event being valid and unconsumed:
>> We flush buffer and queue syn_dropped only when buffer is not empty. So there
>> will be always be atleast one event in buffer that is not consumed and is
>> ofcourse valid.
>>
>> 1.2 About queue is empty
>> If not empty, we do not flush or add syn_dropped at all.
>
> Clock type change is not the only time we queue SYN_DROP, the other time
> is when we fail to handle EVIOCG[type] (during which we remove some
> events from the queue). Queue may be empty when these ioctls are issued.
>
yeah, ideally it should be changed to:
ret = bits_to_user(mem, maxbit, maxlen, p, compat);
if (ret < 0)
+ if (client->head != client->tail)
- evdev_queue_syn_dropped(client);
+ evdev_queue_syn_dropped(client);
Firstly, there is a need to follow protocol of SYN_REPORT as mentioned in
next section.
Unless syn_report does not denote end of a packet,
it is okay without this change too because last event stored would be
syn_report and after flushing, syn_report will still be at the end and
with empty queue too if syn_dropped is queued then we have added syn_report
to not ignore upcoming valid packet.
>>
>> 1.3 About consumption of full or partial packet
>> If client has consumed full packet, then buffer will look like,
>> ... X Y S(consumed) ... X Y S
>> As we always store packets keeping buffer lock and not single events, so
>> there
>> will always be syn_report in the end.
>
> We try to pass full packets to clients, but there is no guarantee. We
> only estimate number of events in device's packet, not guarantee that it
> is correct size.
>
As per documentation, SYN_REPORT should be used to separate packets.
* 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.
So I think, there is a need of below change:
file: input.c
function: input_handle_event:
} else if (dev->num_vals >= dev->max_vals - 2) {
- dev->vals[dev->num_vals++] = input_value_sync;
input_pass_valu
Re: [PATCH] [v8]Input: evdev: fix bug of dropping valid packet after syn_dropped event
Hi Mr. Torokhov,
On Fri, Jan 22, 2016 at 12:47 AM, Dmitry Torokhov
wrote:
> Hi Anoroop,
>
> On Thu, Jan 21, 2016 at 11:07:19PM +0530, Aniroop Mathur wrote:
>> If last event dropped in the old queue was EVi_SYN/SYN_REPORT, then lets
>> generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
>> so that clients would not ignore next valid full packet events.
>>
>> Signed-off-by: Aniroop Mathur
>> ---
>> drivers/input/evdev.c | 46 ++
>> 1 file changed, 34 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index e9ae3d5..821b68a 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
>> *client, unsigned int type)
>> static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> {
>> struct input_event ev;
>> + struct input_event *last_ev;
>> ktime_t time;
>> + unsigned int mask = client->bufsize - 1;
>> +
>> + /* capture last event stored in the buffer */
>> + last_ev = &client->buffer[(client->head - 1) & mask];
>
> I have still the same comment. How do you know that event at last_ev
> position is in fact valid and unconsumed yet event. Also, you need to
> figure out not only if queue contains last SYN event, but also to handle
> the case where the queue is empty and client has consumed either full or
> partial packet at the time you are queueing the drop.
>
Could you please explain what you mean exactly so that I could answer it
properly?
>From what I understood, it seems to me that there is no problem related to
validity, unconsumption, empty queue, full/partial packet.
I would like to explain for clock change request case so that you can know
my understanding for your question.
Clock change request case:
1.1 About last event being valid and unconsumed:
We flush buffer and queue syn_dropped only when buffer is not empty. So there
will be always be atleast one event in buffer that is not consumed and is
ofcourse valid.
1.2 About queue is empty
If not empty, we do not flush or add syn_dropped at all.
1.3 About consumption of full or partial packet
If client has consumed full packet, then buffer will look like,
... X Y S(consumed) ... X Y S
As we always store packets keeping buffer lock and not single events, so there
will always be syn_report in the end.
If syn_dropped is queued here, then queing syn_report is fine.
If client has consumed partial packet, then buffer will look like,
... X(consumed) Y S ... S
If syn_dropped is queued here, then it is fine to queue syn_report because
now new X Y will be reported by driver, and so client will consume new X and Y
and that old X will be replaced with new X and this new packet will be sent by
client to application. Obviously, client never sends partial events to
application and send data packet by packet.
So I do not see any trouble here related to partial or full packet.
> Also please enumerate what changes you done between version n and n+1 so
> I do not have to compare them line by line trying to figure it out
> myself.
>
Difference from v5:
Made a mistake about head index in v5.
Corrected in v8.
evdev_set_clk_type:
- client->packet_head = client->head = client->tail;
+ client->packet_head = client->tail = client->head;
__pass_event:
- client->tail = (client->head - 2) & (client->bufsize - 1);
+ client->head = (client->head - 1) & mask;
+ client->packet_head = client->tail = client->head;
+ __evdev_queue_syn_dropped(client);
v6:
Made change only for clock change and pass event function.
Do not included change for evdev_flush_queue as I was not sure.
But now I think it is okay for evdev_flush_queue also.
As I am not 100% sure, so sent v6 for that.
v7:
please ignore it.
Thanks,
Aniroop Mathur
> Thanks.
>
> --
> Dmitry
[PATCH] [v8]Input: evdev: fix bug of dropping valid packet after syn_dropped event
If last event dropped in the old queue was EVi_SYN/SYN_REPORT, then lets
generate EV_SYN/SYN_REPORT immediately after queing EV_SYN/SYN_DROPPED
so that clients would not ignore next valid full packet events.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 46 ++
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..821b68a 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -156,7 +156,12 @@ static void __evdev_flush_queue(struct evdev_client
*client, unsigned int type)
static void __evdev_queue_syn_dropped(struct evdev_client *client)
{
struct input_event ev;
+ struct input_event *last_ev;
ktime_t time;
+ unsigned int mask = client->bufsize - 1;
+
+ /* capture last event stored in the buffer */
+ last_ev = &client->buffer[(client->head - 1) & mask];
time = client->clk_type == EV_CLK_REAL ?
ktime_get_real() :
@@ -170,13 +175,28 @@ static void __evdev_queue_syn_dropped(struct evdev_client
*client)
ev.value = 0;
client->buffer[client->head++] = ev;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/* drop queue but keep our SYN_DROPPED event */
- client->tail = (client->head - 1) & (client->bufsize - 1);
+ client->tail = (client->head - 1) & mask;
client->packet_head = client->tail;
}
+
+ /*
+* If last packet was completely stored, then queue SYN_REPORT
+* so that clients would not ignore next valid full packet
+*/
+ if (last_ev->type == EV_SYN && last_ev->code == SYN_REPORT) {
+ last_ev->time = ev.time;
+ client->buffer[client->head++] = *last_ev;
+ client->head &= mask;
+ client->packet_head = client->head;
+
+ /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
+ if (unlikely(client->head == client->tail))
+ client->tail = (client->head - 2) & mask;
+ }
}
static void evdev_queue_syn_dropped(struct evdev_client *client)
@@ -218,7 +238,7 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
spin_lock_irqsave(&client->buffer_lock, flags);
if (client->head != client->tail) {
- client->packet_head = client->head = client->tail;
+ client->packet_head = client->tail = client->head;
__evdev_queue_syn_dropped(client);
}
@@ -231,22 +251,24 @@ static int evdev_set_clk_type(struct evdev_client
*client, unsigned int clkid)
static void __pass_event(struct evdev_client *client,
const struct input_event *event)
{
+ unsigned int mask = client->bufsize - 1;
+
client->buffer[client->head++] = *event;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/*
* This effectively "drops" all unconsumed events, leaving
-* EV_SYN/SYN_DROPPED plus the newest event in the queue.
+* EV_SYN/SYN_DROPPED, EV_SYN/SYN_REPORT (if required) and
+* newest event in the queue.
*/
- client->tail = (client->head - 2) & (client->bufsize - 1);
+ client->head = (client->head - 1) & mask;
+ client->packet_head = client->tail = client->head;
+ __evdev_queue_syn_dropped(client);
- client->buffer[client->tail].time = event->time;
- client->buffer[client->tail].type = EV_SYN;
- client->buffer[client->tail].code = SYN_DROPPED;
- client->buffer[client->tail].value = 0;
-
- client->packet_head = client->tail;
+ client->buffer[client->head++] = *event;
+ client->head &= mask;
+ /* No need to check for buffer overflow as it just occurred */
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
--
2.6.2
Re: [PATCH] [v4]Input: evdev - drop partial events after emptying the buffer
Hello Mr. Torokhov,
Could you please help to update about below patch ?
Thanks,
Aniroop Mathur
On Tue, Jan 5, 2016 at 3:26 AM, Aniroop Mathur wrote:
> This patch introduces concept to drop partial events in evdev handler
> itself after emptying the buffer which are dropped by all evdev
> clients in userspace after SYN_DROPPED occurs and this in turn reduces
> evdev client reading requests plus saves memory space filled by partial
> events in evdev handler buffer.
> Also, this patch prevents dropping of full packet by evdev client after
> SYN_DROPPED occurs in case last stored event was SYN_REPORT.
> (like after clock change request)
>
> --
> Please ignore v3.
>
> Signed-off-by: Aniroop Mathur
> ---
> drivers/input/evdev.c | 53
> ++-
> 1 file changed, 40 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index e9ae3d5..15b6eb2 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -58,6 +58,7 @@ struct evdev_client {
> struct list_head node;
> unsigned int clk_type;
> bool revoked;
> + bool drop_pevent; /* specifies if partial events need to be dropped */
> unsigned long *evmasks[EV_CNT];
> unsigned int bufsize;
> struct input_event buffer[];
> @@ -156,7 +157,12 @@ static void __evdev_flush_queue(struct evdev_client
> *client, unsigned int type)
> static void __evdev_queue_syn_dropped(struct evdev_client *client)
> {
> struct input_event ev;
> + struct input_event *prev_ev;
> ktime_t time;
> + unsigned int mask = client->bufsize - 1;
> +
> + /* Store previous event */
> + prev_ev = &client->buffer[(client->head - 1) & mask];
>
> time = client->clk_type == EV_CLK_REAL ?
> ktime_get_real() :
> @@ -170,13 +176,33 @@ static void __evdev_queue_syn_dropped(struct
> evdev_client *client)
> ev.value = 0;
>
> client->buffer[client->head++] = ev;
> - client->head &= client->bufsize - 1;
> + client->head &= mask;
>
> if (unlikely(client->head == client->tail)) {
> /* drop queue but keep our SYN_DROPPED event */
> - client->tail = (client->head - 1) & (client->bufsize - 1);
> + client->tail = (client->head - 1) & mask;
> client->packet_head = client->tail;
> }
> +
> + /*
> +* If last packet is NOT fully stored, set drop_pevent to true to
> +* ignore partial events and if last packet is fully stored, queue
> +* SYN_REPORT so that clients would not ignore next full packet.
> +*/
> + if (prev_ev->type != EV_SYN && prev_ev->code != SYN_REPORT) {
> + client->drop_pevent = true;
> + } else if (prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT) {
> + prev_ev->time = ev.time;
> + client->buffer[client->head++] = *prev_ev;
> + client->head &= mask;
> + client->packet_head = client->head;
> +
> + /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
> + if (unlikely(client->head == client->tail)) {
> + client->tail = (client->head - 2) & mask;
> + client->packet_head = client->tail;
> + }
> + }
> }
>
> static void evdev_queue_syn_dropped(struct evdev_client *client)
> @@ -235,18 +261,8 @@ static void __pass_event(struct evdev_client *client,
> client->head &= client->bufsize - 1;
>
> if (unlikely(client->head == client->tail)) {
> - /*
> -* This effectively "drops" all unconsumed events, leaving
> -* EV_SYN/SYN_DROPPED plus the newest event in the queue.
> -*/
> - client->tail = (client->head - 2) & (client->bufsize - 1);
> -
> - client->buffer[client->tail].time = event->time;
> - client->buffer[client->tail].type = EV_SYN;
> - client->buffer[client->tail].code = SYN_DROPPED;
> - client->buffer[client->tail].value = 0;
> -
> client->packet_head = client->tail;
> + __evdev_queue_syn_dropped(client);
> }
>
> if (event->type == EV_SYN && event->code == SYN_REPORT) {
> @@ -284,6 +300,17 @@ static void evdev_pass_v
Re: [PATCH] [v3]Input: evdev - drop partial events after emptying the buffer
Just now sent v4. Please ignore v3.
On Tue, Jan 5, 2016 at 3:19 AM, kbuild test robot wrote:
> Hi Aniroop,
>
> [auto build test ERROR on input/next]
> [also build test ERROR on v4.4-rc8 next-20160104]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improving the system]
>
> url:
> https://github.com/0day-ci/linux/commits/Aniroop-Mathur/Input-evdev-drop-partial-events-after-emptying-the-buffer/20160105-053003
> base: https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
> config: avr32-atstk1006_defconfig (attached as .config)
> reproduce:
> wget
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=avr32
>
> All errors (new ones prefixed by >>):
>
>drivers/input/evdev.c: In function '__evdev_queue_syn_dropped':
>drivers/input/evdev.c:195: error: request for member 'time' in something
> not a structure or union
>>> drivers/input/evdev.c:196: error: incompatible types in assignment
> --
>/kbuild/src/um/drivers/input/evdev.c: In function
> '__evdev_queue_syn_dropped':
>/kbuild/src/um/drivers/input/evdev.c:195: error: request for member 'time'
> in something not a structure or union
>>> /kbuild/src/um/drivers/input/evdev.c:196: error: incompatible types in
>>> assignment
>
> vim +196 drivers/input/evdev.c
>
>189 * ignore partial events and if last packet is fully stored,
> queue
>190 * SYN_REPORT so that clients would not ignore next full
> packet.
>191 */
>192 if (prev_ev->type != EV_SYN && prev_ev->code != SYN_REPORT) {
>193 client->drop_pevent = true;
>194 } else if (prev_ev->type == EV_SYN && prev_ev->code ==
> SYN_REPORT) {
> > 195 prev_ev.time = ev.time;
> > 196 client->buffer[client->head++] = prev_ev;
>197 client->head &= mask;
>198 client->packet_head = client->head;
>199
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] [v4]Input: evdev - drop partial events after emptying the buffer
This patch introduces concept to drop partial events in evdev handler
itself after emptying the buffer which are dropped by all evdev
clients in userspace after SYN_DROPPED occurs and this in turn reduces
evdev client reading requests plus saves memory space filled by partial
events in evdev handler buffer.
Also, this patch prevents dropping of full packet by evdev client after
SYN_DROPPED occurs in case last stored event was SYN_REPORT.
(like after clock change request)
--
Please ignore v3.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 53 ++-
1 file changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..15b6eb2 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -58,6 +58,7 @@ struct evdev_client {
struct list_head node;
unsigned int clk_type;
bool revoked;
+ bool drop_pevent; /* specifies if partial events need to be dropped */
unsigned long *evmasks[EV_CNT];
unsigned int bufsize;
struct input_event buffer[];
@@ -156,7 +157,12 @@ static void __evdev_flush_queue(struct evdev_client
*client, unsigned int type)
static void __evdev_queue_syn_dropped(struct evdev_client *client)
{
struct input_event ev;
+ struct input_event *prev_ev;
ktime_t time;
+ unsigned int mask = client->bufsize - 1;
+
+ /* Store previous event */
+ prev_ev = &client->buffer[(client->head - 1) & mask];
time = client->clk_type == EV_CLK_REAL ?
ktime_get_real() :
@@ -170,13 +176,33 @@ static void __evdev_queue_syn_dropped(struct evdev_client
*client)
ev.value = 0;
client->buffer[client->head++] = ev;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/* drop queue but keep our SYN_DROPPED event */
- client->tail = (client->head - 1) & (client->bufsize - 1);
+ client->tail = (client->head - 1) & mask;
client->packet_head = client->tail;
}
+
+ /*
+* If last packet is NOT fully stored, set drop_pevent to true to
+* ignore partial events and if last packet is fully stored, queue
+* SYN_REPORT so that clients would not ignore next full packet.
+*/
+ if (prev_ev->type != EV_SYN && prev_ev->code != SYN_REPORT) {
+ client->drop_pevent = true;
+ } else if (prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT) {
+ prev_ev->time = ev.time;
+ client->buffer[client->head++] = *prev_ev;
+ client->head &= mask;
+ client->packet_head = client->head;
+
+ /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
+ if (unlikely(client->head == client->tail)) {
+ client->tail = (client->head - 2) & mask;
+ client->packet_head = client->tail;
+ }
+ }
}
static void evdev_queue_syn_dropped(struct evdev_client *client)
@@ -235,18 +261,8 @@ static void __pass_event(struct evdev_client *client,
client->head &= client->bufsize - 1;
if (unlikely(client->head == client->tail)) {
- /*
-* This effectively "drops" all unconsumed events, leaving
-* EV_SYN/SYN_DROPPED plus the newest event in the queue.
-*/
- client->tail = (client->head - 2) & (client->bufsize - 1);
-
- client->buffer[client->tail].time = event->time;
- client->buffer[client->tail].type = EV_SYN;
- client->buffer[client->tail].code = SYN_DROPPED;
- client->buffer[client->tail].value = 0;
-
client->packet_head = client->tail;
+ __evdev_queue_syn_dropped(client);
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
@@ -284,6 +300,17 @@ static void evdev_pass_values(struct evdev_client *client,
wakeup = true;
}
+ /*
+* drop partial events of last packet but queue SYN_REPORT
+* so that clients would not ignore extra full packet.
+*/
+ if (client->drop_pevent) {
+ if (v->type == EV_SYN && v->code == SYN_REPORT)
+ client->drop_pevent = false;
+ else
+ continue;
+ }
+
event.type = v->type;
event.code = v->code;
even
[PATCH] [v3]Input: evdev - drop partial events after emptying the buffer
This patch introduces concept to drop partial events in evdev handler
itself after emptying the buffer which are dropped by all evdev
clients in userspace after SYN_DROPPED occurs and this in turn reduces
evdev client reading requests plus saves memory space filled by partial
events in evdev handler buffer.
Also, this patch prevents dropping of full packet by evdev client after
SYN_DROPPED occurs in case last stored event was SYN_REPORT.
(like after clock change request)
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 53 ++-
1 file changed, 40 insertions(+), 13 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..2d670e4 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -58,6 +58,7 @@ struct evdev_client {
struct list_head node;
unsigned int clk_type;
bool revoked;
+ bool drop_pevent; /* specifies if partial events need to be dropped */
unsigned long *evmasks[EV_CNT];
unsigned int bufsize;
struct input_event buffer[];
@@ -156,7 +157,12 @@ static void __evdev_flush_queue(struct evdev_client
*client, unsigned int type)
static void __evdev_queue_syn_dropped(struct evdev_client *client)
{
struct input_event ev;
+ struct input_event *prev_ev;
ktime_t time;
+ unsigned int mask = client->bufsize - 1;
+
+ /* Store previous event */
+ prev_ev = &client->buffer[(client->head - 1) & mask];
time = client->clk_type == EV_CLK_REAL ?
ktime_get_real() :
@@ -170,13 +176,33 @@ static void __evdev_queue_syn_dropped(struct evdev_client
*client)
ev.value = 0;
client->buffer[client->head++] = ev;
- client->head &= client->bufsize - 1;
+ client->head &= mask;
if (unlikely(client->head == client->tail)) {
/* drop queue but keep our SYN_DROPPED event */
- client->tail = (client->head - 1) & (client->bufsize - 1);
+ client->tail = (client->head - 1) & mask;
client->packet_head = client->tail;
}
+
+ /*
+* If last packet is NOT fully stored, set drop_pevent to true to
+* ignore partial events and if last packet is fully stored, queue
+* SYN_REPORT so that clients would not ignore next full packet.
+*/
+ if (prev_ev->type != EV_SYN && prev_ev->code != SYN_REPORT) {
+ client->drop_pevent = true;
+ } else if (prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT) {
+ prev_ev.time = ev.time;
+ client->buffer[client->head++] = prev_ev;
+ client->head &= mask;
+ client->packet_head = client->head;
+
+ /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
+ if (unlikely(client->head == client->tail)) {
+ client->tail = (client->head - 2) & mask;
+ client->packet_head = client->tail;
+ }
+ }
}
static void evdev_queue_syn_dropped(struct evdev_client *client)
@@ -235,18 +261,8 @@ static void __pass_event(struct evdev_client *client,
client->head &= client->bufsize - 1;
if (unlikely(client->head == client->tail)) {
- /*
-* This effectively "drops" all unconsumed events, leaving
-* EV_SYN/SYN_DROPPED plus the newest event in the queue.
-*/
- client->tail = (client->head - 2) & (client->bufsize - 1);
-
- client->buffer[client->tail].time = event->time;
- client->buffer[client->tail].type = EV_SYN;
- client->buffer[client->tail].code = SYN_DROPPED;
- client->buffer[client->tail].value = 0;
-
client->packet_head = client->tail;
+ __evdev_queue_syn_dropped(client);
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
@@ -284,6 +300,17 @@ static void evdev_pass_values(struct evdev_client *client,
wakeup = true;
}
+ /*
+* drop partial events of last packet but queue SYN_REPORT
+* so that clients would not ignore extra full packet.
+*/
+ if (client->drop_pevent) {
+ if (v->type == EV_SYN && v->code == SYN_REPORT)
+ client->drop_pevent = false;
+ else
+ continue;
+ }
+
event.type = v->type;
event.code = v->code;
event.value = v->value;
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: evdev - drop partial events after emptying the buffer
On Mon, Jan 4, 2016 at 2:22 PM, Benjamin Tissoires wrote: > On Mon, Jan 4, 2016 at 8:50 AM, Aniroop Mathur wrote: >> On Jan 4, 2016 5:08 AM, "Peter Hutterer" wrote: >>> >>> On Sat, Jan 02, 2016 at 08:39:21PM -0800, Dmitry Torokhov wrote: >>> > On Thu, Dec 31, 2015 at 03:36:47AM +0530, Aniroop Mathur wrote: >>> > > This patch introduces concept to drop partial events in evdev handler >>> > > itself after emptying the buffer which are dropped by all evdev >>> > > clients in userspace after SYN_DROPPED occurs. >>> > > This in turn saves space of partial events in evdev handler buffer >>> > > and reduces evdev client reading requests. >>> > >>> > Let's add a few people who write consumer code to see if this is >>> > something that they consider useful. >>> >>> yeah, it's useful though we already have the code in libevdev to work around >>> this. Still, it reduces the number of events discarde by the client, so >>> it'll be a net >>> plus. but, afaict, there's a bug in this implementation. >>> The doc states: "Client should ignore all events up to and including next >>> SYN_REPORT event". If you drop partial events, you need to have an empty >>> SYN_REPORT after the SYN_DROPPED before you start with full events again. >>> This patch skips that, so after the SYN_DROPPED you have a valid full event >>> that will be ignored by any client currently capable of handling >>> SYN_DROPPED. >>> Example: let's assume a device sending ABS_X/ABS_Y fast enough to cause a >>> SYN_DROPPED, you may have this queue: >>> ABS_X >>> ABS_Y >>> SYN_REPORT >>> ... >>> SYN_DROPPED >>> ABS_Y < partial event >>> SYN_REPORT < client discards up to here, sync state >>> ABS_X >>> ABS_Y >>> SYN_REPORT < first full event after sync >>> >>> With this patch this sequence becomes: >>> ABS_X >>> ABS_Y >>> SYN_REPORT >>> ... >>> SYN_DROPPED >>> [kernel discards ABS_Y + SYN_REPORT as partial event] >>> ABS_X >>> ABS_Y >>> SYN_REPORT <--- client discards up to here, sync state >>><--- there is no event after sync >>> >>> That's a change in kernel behaviour and will make all current clients >>> potentially buggy, you'll really need the empty SYN_REPORT here. >>> >> >> Thank you for your input, Mr. Peter. >> Actually, there is a need to update the documentation as well after this >> patch >> so that clients no more ignore the events after SYN_DROPPED occurs and >> should read the events normally. I skipped updating the documentation in >> this patch as I thought of getting a consent first. >> * SYN_DROPPED: >> - Used to indicate buffer overrun in the evdev client's event queue. >> Client should ignore all events up to and including next SYN_REPORT >> event and query the device (using EVIOCG* ioctls) to obtain its >> current state >> + From kernel version <4.4.x> onwards, clients do no need to ignore >> + events anymore and should read normally as there will be no >> + partial events after SYN_DROPPED occurs. > > Hi Aniroop, > > this just won't do. As Peter said, there are current implementation of > SYN_DROPPED around, which ignore the events until the next SYN_REPORT. > If you change the protocol by updating the doc, you will just break > existing userspace which has not included a check on the kernel > version (and honestly, checking the kernel version from the userspace > point of view is just a nightmare when distributions start backporting > changes here and there). > > The kernel rule is "do not break userspace", so we can not accept this. > Peter suggested you just add an empty SYN_REPORT after SYN_DROPPED > which would solve the whole problem: clients already handling > SYN_DROPPED will receive the next valid event, and those who don't (or > which will be updated) will not have to do anything more. > > The only cons I can think of is that multitouch protocol A will be a > pain to handle with this empty SYN_REPORT if you do not handle the > SYN_DROPPED as per the doc. > But on the other hand, if you have a MT protocol A device, you are > screwed anyway because you need mtdev and so let's use libevdev at > this point. > >> >> As fa
[PATCH] [v2]Input: evdev - drop partial events after emptying the buffer
This patch introduces concept to drop partial events in evdev handler
itself after emptying the buffer which are dropped by all evdev
clients in userspace after SYN_DROPPED occurs.
This in turn saves space of partial events in evdev handler buffer
and reduces evdev client reading requests.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 56 ++-
1 file changed, 47 insertions(+), 9 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..0a5ead8 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -58,6 +58,7 @@ struct evdev_client {
struct list_head node;
unsigned int clk_type;
bool revoked;
+ bool drop_pevent; /* specifies whether partial events need to be
dropped */
unsigned long *evmasks[EV_CNT];
unsigned int bufsize;
struct input_event buffer[];
@@ -192,6 +193,7 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
{
unsigned long flags;
unsigned int clk_type;
+ struct input_event ev;
switch (clkid) {
@@ -218,8 +220,26 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
spin_lock_irqsave(&client->buffer_lock, flags);
if (client->head != client->tail) {
- client->packet_head = client->head = client->tail;
+
+ /* Store previous event */
+ client->head--;
+ client->head &= client->bufsize - 1;
+ ev = client->buffer[client->head];
+
+ client->packet_head = client->head = client->tail = 0;
__evdev_queue_syn_dropped(client);
+
+ /*
+* If last packet is fully stored, queue SYN_REPORT
+* so that clients do not ignore next full packet.
+* And if last packet is NOT fully stored,
+* set drop_pevent to true to ignore partial events.
+*/
+ if (ev.type == EV_SYN && ev.code == SYN_REPORT) {
+ ev.time = client->buffer[client->head - 1].time;
+ client->buffer[client->head++] = ev;
+ } else if (ev.type != EV_SYN && ev.code != SYN_REPORT)
+ client->drop_pevent = true;
}
spin_unlock_irqrestore(&client->buffer_lock, flags);
@@ -236,17 +256,27 @@ static void __pass_event(struct evdev_client *client,
if (unlikely(client->head == client->tail)) {
/*
-* This effectively "drops" all unconsumed events, leaving
-* EV_SYN/SYN_DROPPED plus the newest event in the queue.
+* This effectively "drops" all unconsumed events, storing
+* EV_SYN/SYN_DROPPED and the newest event in the queue but
+* only if it is EV_SYN/SYN_REPORT so that clients can read
+* the next full event packet. And set drop_pevent to true
+* if last event packet is not stored completely in buffer
+* to ignore upcoming partial events.
*/
- client->tail = (client->head - 2) & (client->bufsize - 1);
- client->buffer[client->tail].time = event->time;
- client->buffer[client->tail].type = EV_SYN;
- client->buffer[client->tail].code = SYN_DROPPED;
- client->buffer[client->tail].value = 0;
+ client->tail = client->head = client->packet_head;
- client->packet_head = client->tail;
+ client->buffer[client->head].time = event->time;
+ client->buffer[client->head].type = EV_SYN;
+ client->buffer[client->head].code = SYN_DROPPED;
+ client->buffer[client->head].value = 0;
+ client->head = (client->head + 1) & (client->bufsize -1);
+
+ if (event->type == EV_SYN && event->code == SYN_REPORT) {
+ client->buffer[client->head++] = *event;
+ client->head &= client->bufsize - 1;
+ } else if (event->type != EV_SYN && event->code != SYN_REPORT)
+ client->drop_pevent = true;
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
@@ -284,6 +314,14 @@ static void evdev_pass_values(struct evdev_client *client,
wakeup = true;
}
+ /* drop partial events of last packet
Re: Re: [PATCH] Input: evdev - drop partial events after emptying the buffer
On Jan 4, 2016 5:08 AM, "Peter Hutterer" wrote:
>
> On Sat, Jan 02, 2016 at 08:39:21PM -0800, Dmitry Torokhov wrote:
> > On Thu, Dec 31, 2015 at 03:36:47AM +0530, Aniroop Mathur wrote:
> > > This patch introduces concept to drop partial events in evdev handler
> > > itself after emptying the buffer which are dropped by all evdev
> > > clients in userspace after SYN_DROPPED occurs.
> > > This in turn saves space of partial events in evdev handler buffer
> > > and reduces evdev client reading requests.
> >
> > Let's add a few people who write consumer code to see if this is
> > something that they consider useful.
>
> yeah, it's useful though we already have the code in libevdev to work around
> this. Still, it reduces the number of events discarde by the client, so it'll
> be a net
> plus. but, afaict, there's a bug in this implementation.
> The doc states: "Client should ignore all events up to and including next
> SYN_REPORT event". If you drop partial events, you need to have an empty
> SYN_REPORT after the SYN_DROPPED before you start with full events again.
> This patch skips that, so after the SYN_DROPPED you have a valid full event
> that will be ignored by any client currently capable of handling
> SYN_DROPPED.
> Example: let's assume a device sending ABS_X/ABS_Y fast enough to cause a
> SYN_DROPPED, you may have this queue:
> ABS_X
> ABS_Y
> SYN_REPORT
> ...
> SYN_DROPPED
> ABS_Y < partial event
> SYN_REPORT < client discards up to here, sync state
> ABS_X
> ABS_Y
> SYN_REPORT < first full event after sync
>
> With this patch this sequence becomes:
> ABS_X
> ABS_Y
> SYN_REPORT
> ...
> SYN_DROPPED
> [kernel discards ABS_Y + SYN_REPORT as partial event]
> ABS_X
> ABS_Y
> SYN_REPORT <--- client discards up to here, sync state
><--- there is no event after sync
>
> That's a change in kernel behaviour and will make all current clients
> potentially buggy, you'll really need the empty SYN_REPORT here.
>
Thank you for your input, Mr. Peter.
Actually, there is a need to update the documentation as well after this patch
so that clients no more ignore the events after SYN_DROPPED occurs and
should read the events normally. I skipped updating the documentation in
this patch as I thought of getting a consent first.
* SYN_DROPPED:
- Used to indicate buffer overrun in the evdev client's event queue.
Client should ignore all events up to and including next SYN_REPORT
event and query the device (using EVIOCG* ioctls) to obtain its
current state
+ From kernel version <4.4.x> onwards, clients do no need to ignore
+ events anymore and should read normally as there will be no
+ partial events after SYN_DROPPED occurs.
As far as I've worked on client codes, this client code change is easy and
even if some clients miss to update the code then it seems not much of
a problem because 8 packets are already dropped so an additional packet
would not cause any trouble in case of buffer overrun.
Regards,
Aniroop Mathur
> > >
> > > Signed-off-by: Aniroop Mathur
> > > ---
> > > drivers/input/evdev.c | 49
> > > +
> > > 1 file changed, 45 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > > index e9ae3d5..e7b612e 100644
> > > --- a/drivers/input/evdev.c
> > > +++ b/drivers/input/evdev.c
> > > @@ -58,6 +58,7 @@ struct evdev_client {
> > > struct list_head node;
> > > unsigned int clk_type;
> > > bool revoked;
> > > + bool drop_pevent; /* specifies whether partial events need to be
> > > dropped */
> > > unsigned long *evmasks[EV_CNT]; > > unsigned int bufsize;
> > > struct input_event buffer[];
> > > @@ -192,6 +193,7 @@ static int evdev_set_clk_type(struct evdev_client
> > > *client, unsigned int clkid)
> > > {
> > > unsigned long flags;
> > > unsigned int clk_type;
> > > + struct input_event *ev;
> > >
> > > switch (clkid) {
> > >
> > > @@ -218,6 +220,17 @@ static int evdev_set_clk_type(struct evdev_client
> > > *client, unsigned int clkid)
> > > spin_lock_irqsave(&client->buffer_lock, flags);
> > >
> > > if (client->head != client->tail) {
> > > +
> > > + /*
> >
[PATCH] Input: evdev - Avoid duplicate checking of empty SYN_REPORT
Input core already drops empty syn_report event so there is no need
to check again for dropping empty syn_report event in evdev handler
for all clients on every new event.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..6e242b7 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -276,13 +276,8 @@ static void evdev_pass_values(struct evdev_client *client,
if (__evdev_is_filtered(client, v->type, v->code))
continue;
- if (v->type == EV_SYN && v->code == SYN_REPORT) {
- /* drop empty SYN_REPORT */
- if (client->packet_head == client->head)
- continue;
-
+ if (v->type == EV_SYN && v->code == SYN_REPORT)
wakeup = true;
- }
event.type = v->type;
event.code = v->code;
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Input: evdev - drop partial events after emptying the buffer
This patch introduces concept to drop partial events in evdev handler
itself after emptying the buffer which are dropped by all evdev
clients in userspace after SYN_DROPPED occurs.
This in turn saves space of partial events in evdev handler buffer
and reduces evdev client reading requests.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 49 +
1 file changed, 45 insertions(+), 4 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..e7b612e 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -58,6 +58,7 @@ struct evdev_client {
struct list_head node;
unsigned int clk_type;
bool revoked;
+ bool drop_pevent; /* specifies whether partial events need to be
dropped */
unsigned long *evmasks[EV_CNT];
unsigned int bufsize;
struct input_event buffer[];
@@ -192,6 +193,7 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
{
unsigned long flags;
unsigned int clk_type;
+ struct input_event *ev;
switch (clkid) {
@@ -218,6 +220,17 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
spin_lock_irqsave(&client->buffer_lock, flags);
if (client->head != client->tail) {
+
+ /*
+* Set drop_pevent to true if last event packet is
+* not stored completely in buffer.
+*/
+ client->head--;
+ client->head &= client->bufsize - 1;
+ ev = &client->buffer[client->head];
+ if (!(ev->type == EV_SYN && ev->code == SYN_REPORT))
+ client->drop_pevent = true;
+
client->packet_head = client->head = client->tail;
__evdev_queue_syn_dropped(client);
}
@@ -228,31 +241,51 @@ static int evdev_set_clk_type(struct evdev_client
*client, unsigned int clkid)
return 0;
}
-static void __pass_event(struct evdev_client *client,
+static bool __pass_event(struct evdev_client *client,
const struct input_event *event)
{
+ struct input_event *prev_ev;
+
client->buffer[client->head++] = *event;
client->head &= client->bufsize - 1;
if (unlikely(client->head == client->tail)) {
/*
-* This effectively "drops" all unconsumed events, leaving
-* EV_SYN/SYN_DROPPED plus the newest event in the queue.
+* This effectively "drops" all unconsumed events, storing
+* EV_SYN/SYN_DROPPED and the newest event in the queue but
+* only if it is not part of partial packet.
+* Set drop_pevent to true if last event packet is not stored
+* completely in buffer and set to false if SYN_REPORT occurs.
*/
+
client->tail = (client->head - 2) & (client->bufsize - 1);
+ prev_ev = &client->buffer[client->tail];
+ if (!(prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT)) {
+ client->drop_pevent = true;
+ client->head--;
+ client->head &= client->bufsize - 1;
+ }
+
client->buffer[client->tail].time = event->time;
client->buffer[client->tail].type = EV_SYN;
client->buffer[client->tail].code = SYN_DROPPED;
client->buffer[client->tail].value = 0;
client->packet_head = client->tail;
+
+ if (event->type == EV_SYN && event->code == SYN_REPORT) {
+ client->drop_pevent = false;
+ return true;
+ }
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
client->packet_head = client->head;
kill_fasync(&client->fasync, SIGIO, POLL_IN);
}
+
+ return false;
}
static void evdev_pass_values(struct evdev_client *client,
@@ -284,10 +317,18 @@ static void evdev_pass_values(struct evdev_client *client,
wakeup = true;
}
+ /* drop partial events until SYN_REPORT occurs */
+ if (client->drop_pevent) {
+ if (v->type == EV_SYN && v->code == SYN_REPORT)
+ client->drop_pevent = false;
+ continue;
+ }
+
event.type = v->type;
event.code = v->code;
Re: [PATCH] Input: evdev: avoid storing newest SYN_REPORT when dropping all events
Hello Mr. Torokhov, On Tue, Dec 29, 2015 at 5:11 AM, Dmitry Torokhov wrote: > Hi Aniroop, > > On Thu, Dec 24, 2015 at 11:25 AM, Aniroop Mathur wrote: >> The newest event can be SYN_REPORT in case of dropping all old events when >> buffer is full, but as now there is no device data available to read so >> lets drop SYN_REPORT as well and store only SYN_DROPPED. >> > > We can not drop the SYN_REPORT event in this case as userspace needs > it to determine if the events that are coming after SYN_DROPPED are > part of a full packet or if they belong to a partial packet. From > Documentation/input/event-codes.txt: > > * SYN_DROPPED: > - Used to indicate buffer overrun in the evdev client's event queue. > Client should ignore all events up to and including next SYN_REPORT > event and query the device (using EVIOCG* ioctls) to obtain its > current state. > Okay I see. In case of buffer overrun, how about handling the partial packet case in evdev handler itself and free evdev client from handling it ? Normally, evdev buffer contain only full packets. But in case of dropping events, buffer would contain a partial packet almost all times and hence evdev client would have to handle the case of partial packet and as documentation says, client will ignore the events until new packet starts. So as a new protocol, after buffer overrun occurs, evdev handler can ignore the events until SYN_REPORT occurs and then start storing events. This way buffer will contain full packet as always and reading would be simplified for evdev clients. Accordingly, document can also be updated. Please let me know your opinion about it. Thanks and Regards, Aniroop Mathur > Thanks. > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Input: evdev: avoid storing newest SYN_REPORT when dropping all events
The newest event can be SYN_REPORT in case of dropping all old events when
buffer is full, but as now there is no device data available to read so
lets drop SYN_REPORT as well and store only SYN_DROPPED.
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..e270219 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -228,7 +228,7 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
return 0;
}
-static void __pass_event(struct evdev_client *client,
+static int __pass_event(struct evdev_client *client,
const struct input_event *event)
{
client->buffer[client->head++] = *event;
@@ -247,12 +247,24 @@ static void __pass_event(struct evdev_client *client,
client->buffer[client->tail].value = 0;
client->packet_head = client->tail;
+
+ /*
+* Do not store SYN_REPORT as there is no device data in buffer
+* and return to avoid wakeup and changing packet_head.
+*/
+ if (event->type == EV_SYN && event->code == SYN_REPORT) {
+ client->head--;
+ client->head &= client->bufsize - 1;
+ return 1;
+ }
}
if (event->type == EV_SYN && event->code == SYN_REPORT) {
client->packet_head = client->head;
kill_fasync(&client->fasync, SIGIO, POLL_IN);
}
+
+ return 0;
}
static void evdev_pass_values(struct evdev_client *client,
@@ -287,7 +299,8 @@ static void evdev_pass_values(struct evdev_client *client,
event.type = v->type;
event.code = v->code;
event.value = v->value;
- __pass_event(client, &event);
+ if (__pass_event(client, &event))
+ wakeup = false;
}
spin_unlock(&client->buffer_lock);
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
On Tue, Dec 15, 2015 at 1:23 AM, Thomas Gleixner wrote: > On Tue, 15 Dec 2015, Aniroop Mathur wrote: >> On Sun, Dec 13, 2015 at 3:17 PM, Clemens Ladisch wrote: >> > Aniroop Mathur wrote: >> >>> 2. If we use msleep(40), is it possible that process could wake up after >> >>> 60 ms or 70 ms or 100 ms or more ? >> > >> > Yes, if lots of other processes compete for the CPU. >> >> You mean to say "yes" for process competing for changing state >> from "waiting" to "ready" or "ready" to "running" ? > > There is no state ready. We change from [un]interruptible to running, > but that's just the wakeup by the timer callback. When the task gets > on the CPU is a different question. > >> Regarding above, could you please help to confirm below things? >> 1. Using msleep(40) with HZ=1000 (1ms), process can still be in >> "waiting" state and will not move to "ready" state even after 42,45 or 50 ms. >> Right ? > > kernel/time/timer.c:apply_slack() > >> 2. Using usleep_range(4, 41000), it is guaranteed that process will >> change its state from waiting to ready state before or at 41 ms. >> Right ? > > 1) It is supposed to be woken up by the timer in that range, but there >is no guarantee. Depends also on your kernel configuration and >hardware capabilities. > > 2) The state changes from [un]interruptible to running. And again >that does not tell you when it gets on the CPU. > >> Regarding usleep_range disadvatage: >> 1. Usleep_range has a disadvantage that it does not allow the system to >> do optimal timer batching for power savings. Except that, Is there any >> other disadvantage? > > Higher CPU usage. > Really appreciate your input, Mr. Thomas. Suddenly today, I thought again and realised than Less Power Saving and High CPU Usage are disadvantages of usleep_range as a whole and not only when used for delays more than 20ms. I hope my understanding is right. If yes, Could you please help to confirm my above understanding ? and let me know if there is any disadvantage "specifically for large delays" ? Thanks again for your support on this. Regards, Aniroop Mathur >> 2. Is the impact of optimal timer batching in systems like android or ubuntu >> with moderate cpu speed significant or it can be negligible also? >> To understand the impact better, it would be really appreciating if you could >> kindly explain in detail with some case and data input. > > http://bfy.tw/3HaV > http://bfy.tw/3Han > > > Thanks, > > tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
On Sun, Dec 13, 2015 at 3:17 PM, Clemens Ladisch wrote: > Aniroop Mathur wrote: >>> 1. If we choose usleep_range to acheive accuracy, are cpu and power usage >>> of any concern in real sense ? > > That depends. The effects are worse if the CPU is sleeping, and slow to > wake up. > > But that's _your_ job to decide. (And you know your hardware better.) > Umm... could you please elaborate it ? >>> 2. If we use msleep(40), is it possible that process could wake up after >>> 60 ms or 70 ms or 100 ms or more ? > > Yes, if lots of other processes compete for the CPU. > > You mean to say "yes" for process competing for changing state from "waiting" to "ready" or "ready" to "running" ? Regarding above, could you please help to confirm below things? 1. Using msleep(40) with HZ=1000 (1ms), process can still be in "waiting" state and will not move to "ready" state even after 42,45 or 50 ms. Right ? 2. Using usleep_range(4, 41000), it is guaranteed that process will change its state from waiting to ready state before or at 41 ms. Right ? Regarding usleep_range disadvatage: 1. Usleep_range has a disadvantage that it does not allow the system to do optimal timer batching for power savings. Except that, Is there any other disadvantage ? 2. Is the impact of optimal timer batching in systems like android or ubuntu with moderate cpu speed significant or it can be negligible also? To understand the impact better, it would be really appreciating if you could kindly explain in detail with some case and data input. Regards, Aniroop Mathur > Regards, > Clemens -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
Sorry for multiple emails as those were not in plain text so ignored by linux-kernel@vger.kernel.org Please consider this email for reply. On Wed, Dec 9, 2015 at 2:17 AM, Thomas Gleixner wrote: > On Tue, 8 Dec 2015, Aniroop Mathur wrote: >> On Tue, Dec 8, 2015 at 4:18 PM, Thomas Gleixner wrote: >> > The initialization process is hardly something critical, so why would >> > the delay need to be precise? What's the point of having data 10ms >> > earlier? >> >> As I know, the chip initialisation process is critical. >> Consider the case for an android mobile phone. When the phone is >> resumed from suspend state, all the earlier enabled devices need to >> be re-initialised. Normally, we have two sleeps during device >> initialisation and we need to re-initialize more than 25 devices on >> board. So if single msleep delays by 10 ms, then the android phone >> resume is delayed by 10*2*25 = 500 ms, which is quite a big time. >> >> Also more importantly, during booting the phone as well, if every >> device sleeps for extra 20 ms and we have to probe 25 devices, >> booting is delayed by 500 ms. > > You are optimizing for something which is simply stupid. WHY do you > need to (re)initialize all devices before you can do something useful? > > Just because Android is implemented that way? That's silly. > > If you really care about boot time / user experience you initialize > only the really important drivers in the boot/resume process and > initialize the reset in the background. It does not matter whether the > temperatur app shows the updated value one second later or not, but it > matters for the user that the GUI is functional. > In case of android suspend, first lcd and touch turns off, then system devices are turned off. The same process I found in laptop as well. I could see clearly, first lcd went off and then wifi light was off, bluetooth light was off, mouse light was off. In resume, reverse process is followed in both. So LCD is turned on at last. In my opinion, this makes sense as we do not want user to use the gui or perform any operation untill the system is ready to operate. So with this sequence we are able to save good time using usleep_range. In case of booting, as almost all devices are embedded in the android phone, so probe is called during booting. In every probe, I have seen the prevention code of checking chipID or some other prevention code. For this, chip is powered on and initialized, next i2c operation is performed to read slave address. If slave address does not match or i2c operation fails, then probe fails and chip is de-initialized. Also, if probe success, chip is de-initialized. So basically in probe, chip is always initialized and de-initialized finally. And in initialization part, we need to add delays. So saving time in initialization part can save good time in booting till home screen. >> My point is to use single consistent sleep api in driver code >> instead of two as we need to use it many places in a single driver >> code. This way, the code looks better. > > It's not about better. It's about using the right interfaces for the > right job. usleep_range() and msleep() use different facilities. As I > explained before: If you need a precise limit, use usleep_range(). If > not use msleep(). > Sure. I understand this part. However, my main concern is still not resolved clearly. Could you please update the answer to my earlier two queries: 1. If we choose usleep_range to acheive accuracy, are cpu and power usage of any concern in real sense ? 2. If we use msleep(40), is it possible that process could wake up after 60 ms or 70 ms or 100 ms or more ? BR, Aniroop Mathur > Thanks, > > tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
On Dec 9, 2015 2:18 AM, "Thomas Gleixner" wrote: > > On Tue, 8 Dec 2015, Aniroop Mathur wrote: > > On Tue, Dec 8, 2015 at 4:18 PM, Thomas Gleixner wrote: > > > The initialization process is hardly something critical, so why would > > > the delay need to be precise? What's the point of having data 10ms > > > earlier? > > > > As I know, the chip initialisation process is critical. > > Consider the case for an android mobile phone. When the phone is > > resumed from suspend state, all the earlier enabled devices need to > > be re-initialised. Normally, we have two sleeps during device > > initialisation and we need to re-initialize more than 25 devices on > > board. So if single msleep delays by 10 ms, then the android phone > > resume is delayed by 10*2*25 = 500 ms, which is quite a big time. > > > > Also more importantly, during booting the phone as well, if every > > device sleeps for extra 20 ms and we have to probe 25 devices, > > booting is delayed by 500 ms. > > You are optimizing for something which is simply stupid. WHY do you > need to (re)initialize all devices before you can do something useful? > > Just because Android is implemented that way? That's silly. > > If you really care about boot time / user experience you initialize > only the really important drivers in the boot/resume process and > initialize the reset in the background. It does not matter whether the > temperatur app shows the updated value one second later or not, but it > matters for the user that the GUI is functional. > In case of android suspend, first lcd and touch turns off, then system devices are turned off. The same process I found in laptop as well. I could see clearly, first lcd went off and then wifi light was off, bluetooth light was off, mouse light was off. In resume, reverse process is followed in both. So LCD is turned on in last. In my opinion, this makes sense as we do not want user to use the gui or perform any operation untill the system is ready to operate. In case of booting, almost all devices are embedded in the android phone, so probe is called during booting. In every probe I have seen the prevention code of checking chipID or some other prevention code. For this, chip is powered on and initialized, next i2c operation is performed to read slave address. If slave address does not match or i2c operation fails, probe fails and chip is de-initialized. Also, if probe success, chip is de-initialized. So basically in probe, chip is always initialized and de-initialized finally. And in initialization part, we need to add delays. And so saving time in initialization part can save time of booting till home screen. > > My point is to use single consistent sleep api in driver code > > instead of two as we need to use it many places in a single driver > > code. This way, the code looks better. > > It's not about better. It's about using the right interfaces for the > right job. usleep_range() and msleep() use different facilities. As I > explained before: If you need a precise limit, use usleep_range(). If > not use msleep(). > Sure. I understand this part. However, my main concern is still incomplete. Could you please provide the answer to my earlier two queries: 1. If we choose usleep_range to acheive accuracy, are cpu and power usage of any concern? 2. If we use msleep(40), is it possible that process could wake up at 60 ms or 70 ms or 100 ms or more ? BR, Aniroop Mathur > Thanks, > > tglx
Re: Ques: [kernel/time/*] Is there any disadvantage in using usleep_range for more than 20ms delay ?
On Tue, Dec 8, 2015 at 2:33 PM, Clemens Ladisch wrote: > Aniroop Mathur wrote: >> As in the kernel documentation, it is mentioned to use msleep for >> 10ms+ delay, I am confused whether there would be any disadvantage in >> using usleep_range for higher delays values because normally drivers >> have variety of delays used (2, 10, 20, 40, 100, 500 ms). >> >> So, could you please help to confirm that if we use usleep_range for >> inserting delays greater than 20 ms, would it be harmful or beneficial >> or does not make any difference at all ? > > As the documentation told you, usleep_range() is likely to require > a separate interrupt, while msleep() is likely to round to some other, > already-scheduled interrupt. The former is possibly harmful regarding > CPU and power usage; you have to balance it against your need for > accuracy. > Thank you for the answer! usleep_range will generate an interrupt to achieve accuracy. However, would that be considered as harmful or a disadvantage ? Would the power usage and cpu really substantial ? PS: I have added my more concern and explanation in another email thread whose subject misses u in usleep_range, by mistake. Added you in it as well. > (And usleep_range() has a 32-bit nanosecond limit on 32-bit > architectures.) > > > Regards, > Clemens -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
On Tue, Dec 8, 2015 at 4:18 PM, Thomas Gleixner wrote: > On Tue, 8 Dec 2015, Aniroop Mathur wrote: >> On Tue, Dec 8, 2015 at 12:07 AM, Thomas Gleixner wrote: >> > The real question is how precise must your delay be? If the delay >> > needs to be precise within the min/max sleep time limits, then >> > usleep_range() is probably the way to go. If the delay can be >> > imprecise then using msleep() is the right way because that lets the >> > kernel batch timers for power saving purposes. >> >> Thank you for the answer ! >> Normally, we insert delays in driver while enabling the chip. >> So here usleep_range seems to service better as we do not want to delay >> the initialisation process of chip and make it ready to generate data, >> especially for faster devices like sensor. > > The initialization process is hardly something critical, so why would > the delay need to be precise? What's the point of having data 10ms > earlier? > As I know, the chip initialisation process is critical. Consider the case for an android mobile phone. When the phone is resumed from suspend state, all the earlier enabled devices need to be re-initialised. Normally, we have two sleeps during device initialisation and we need to re-initialize more than 25 devices on board. So if single msleep delays by 10 ms, then the android phone resume is delayed by 10*2*25 = 500 ms, which is quite a big time. Also more importantly, during booting the phone as well, if every device sleeps for extra 20 ms and we have to probe 25 devices, booting is delayed by 500 ms. So, for above cases, would the power and cpu usage really significant ? Will it affect any factor noticeably if we use usleep_range ? >> One last thing, >> Considering HZ=100, would the power saving be same if we set the >> range in usleep_range equivalent to msleep ? >> For example: msleep (33) and usleep_range(33000, 4) >> So for such case, would both have same impact on power saving ? > > Probably, but what's the point of doing that? > My point is to use single consistent sleep api in driver code instead of two as we need to use it many places in a single driver code. This way, the code looks better. Secondly, I know that usleep_range will definitely wake the process after max time. But what about msleep ? If we use msleep(40), is it guaranteed that process will wake definitely after 50 ms or is it also possible that process can wake after 60 or 70 or even 100 ms or more ? (HZ=100) Best Regards, Aniroop Mathur > Thanks, > > tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
On Tue, Dec 8, 2015 at 12:07 AM, Thomas Gleixner wrote: > On Mon, 7 Dec 2015, Aniroop Mathur wrote: >> As in the kernel documentation, it is mentioned to use msleep for >> 10ms+ delay, I am confused whether there would be any disadvantage in >> using usleep_range for higher delays values because normally drivers >> have variety of delays used (2, 10, 20, 40, 100, 500 ms). > > The real question is how precise must your delay be? If the delay > needs to be precise within the min/max sleep time limits, then > usleep_range() is probably the way to go. If the delay can be > imprecise then using msleep() is the right way because that lets the > kernel batch timers for power saving purposes. > Thank you for the answer ! Normally, we insert delays in driver while enabling the chip. So here usleep_range seems to service better as we do not want to delay the initialisation process of chip and make it ready to generate data, especially for faster devices like sensor. One last thing, Considering HZ=100, would the power saving be same if we set the range in usleep_range equivalent to msleep ? For example: msleep (33) and usleep_range(33000, 4) So for such case, would both have same impact on power saving ? Best Regards, Aniroop Mathur > Thanks, > > tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Ques: [kernel/time/*] Is there any disadvantage in using usleep_range for more than 20ms delay ?
Dear Mr. John and Mr. Thomas, Greetings of the day !! This is Aniroop Mathur working on sensor kernel drivers for last 3 years. Recently, In my driver code, I have been changing msleep to usleep_range. But I got stuck at one point and could not find proper answer on internet. Could you please help to answer my query as mentioned below ? >From the kernel documentation, I understood that it is better to use usleep_range for 10 us - 20 ms delay. For delays 10ms+, it is mentioned to use msleep. If my understanding is right and considering HZ=100, Even for 33 ms delay, msleep would sleep for 40 ms, while usleep_range would sleep for 33 ms as desired. And for 40 ms delay, msleep and usleep_range both will wake up at desired time. Right ? As in the kernel documentation, it is mentioned to use msleep for 10ms+ delay, I am confused whether there would be any disadvantage in using usleep_range for higher delays values because normally drivers have variety of delays used (2, 10, 20, 40, 100, 500 ms). So, could you please help to confirm that if we use usleep_range for inserting delays greater than 20 ms, would it be harmful or beneficial or does not make any difference at all ? Thanks in advance ! Regards, Aniroop Mathur -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Ques: [kernel/time/*] Is there any disadvantage in using sleep_range for more than 20ms delay ?
Dear Mr. John and Mr. Thomas, Greetings of the day !! This is Aniroop Mathur working on sensor kernel drivers for last 3 years. Recently, In my driver code, I have been changing msleep to usleep_range. But I got stuck at one point and could not find proper answer on internet. Could you please help to answer my query as mentioned below ? >From the kernel documentation, I understood that it is better to use usleep_range for 10 us - 20 ms delay. For delays 10ms+, it is mentioned to use msleep. If my understanding is right and considering HZ=100, Even for 33 ms delay, msleep would sleep for 40 ms, while usleep_range would sleep for 33 ms as desired. And for 40 ms delay, msleep and usleep_range both will wake up at desired time. Right ? As in the kernel documentation, it is mentioned to use msleep for 10ms+ delay, I am confused whether there would be any disadvantage in using usleep_range for higher delays values because normally drivers have variety of delays used (2, 10, 20, 40, 100, 500 ms). So, could you please help to confirm if there could be some problem in using usleep_range for higher delay values ? Thanks in advance ! Regards, Aniroop Mathur -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Evdev: Fix bug in checking duplicate clock change request
On Fri, Oct 30, 2015 at 6:34 PM, Aniroop Mathur
wrote:
>
> Hello Mr. Torokhov,
>
> On Fri, Oct 30, 2015 at 4:45 PM, Dmitry Torokhov
> wrote:
> > Hi Aniroop,
> >
> > On Thu, Oct 29, 2015 at 01:38:32AM +0530, Aniroop Mathur wrote:
> >> From: Aniroop Mathur
> >>
> >> clk_type and clkid stores different predefined clock identification
> >> values so they cannot be compared for checking duplicate clock change
> >> request. Therefore, lets fix it to avoid unexpected results.
> >
> > Good catch!
> >
>
> Thank you :)
>
> >>
> >> Signed-off-by: Aniroop Mathur
> >> Signed-off-by: Aniroop Mathur
> >
> > You only need one signoff, which one should I keep? The Samsung one?
> >
>
> Yes, Samsung one.
>
> >> ---
> >> drivers/input/evdev.c | 10 ++
> >> 1 file changed, 6 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> >> index 08d4964..0d40f6f 100644
> >> --- a/drivers/input/evdev.c
> >> +++ b/drivers/input/evdev.c
> >> @@ -56,7 +56,7 @@ struct evdev_client {
> >> struct fasync_struct *fasync;
> >> struct evdev *evdev;
> >> struct list_head node;
> >> - int clk_type;
> >> + unsigned int clk_type;
> >> bool revoked;
> >> unsigned int bufsize;
> >> struct input_event buffer[];
> >> @@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct
> >> evdev_client *client)
> >> static int evdev_set_clk_type(struct evdev_client *client, unsigned int
> >> clkid)
> >> {
> >> unsigned long flags;
> >> -
> >> - if (client->clk_type == clkid)
> >> - return 0;
> >> + unsigned int prev_clk_type = client->clk_type;
> >
> > I prefer the other way around: convert to a temp and then compare. I;ll
> > fix it up on my side.
> >
>
> I thought that was the only better way of all.
> I am not sure what exactly you're referring. I guess I need little
> more elaboration here or anyways, I'll check it up up once you apply
> it.
Ahhh okay... I got the other way around.
(Filling client->clk_type in the end instead)
>
>
>
> >>
> >> switch (clkid) {
> >>
> >> @@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client
> >> *client, unsigned int clkid)
> >> return -EINVAL;
> >> }
> >>
> >> + /* No need to flush if clk_type is same as before */
> >> + if (client->clk_type == prev_clk_type)
> >> + return 0;
> >> +
> >> /*
> >>* Flush pending events and queue SYN_DROPPED event,
> >>* but only if the queue is not empty.
> >> --
> >> 2.6.2
> >>
> >
> > Thanks.
> >
> > --
> > Dmitry
>
> Regards,
> Aniroop
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Evdev: Fix bug in checking duplicate clock change request
Hello Mr. Torokhov,
On Fri, Oct 30, 2015 at 4:45 PM, Dmitry Torokhov
wrote:
> Hi Aniroop,
>
> On Thu, Oct 29, 2015 at 01:38:32AM +0530, Aniroop Mathur wrote:
>> From: Aniroop Mathur
>>
>> clk_type and clkid stores different predefined clock identification
>> values so they cannot be compared for checking duplicate clock change
>> request. Therefore, lets fix it to avoid unexpected results.
>
> Good catch!
>
Thank you :)
>>
>> Signed-off-by: Aniroop Mathur
>> Signed-off-by: Aniroop Mathur
>
> You only need one signoff, which one should I keep? The Samsung one?
>
Yes, Samsung one.
>> ---
>> drivers/input/evdev.c | 10 ++
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index 08d4964..0d40f6f 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -56,7 +56,7 @@ struct evdev_client {
>> struct fasync_struct *fasync;
>> struct evdev *evdev;
>> struct list_head node;
>> - int clk_type;
>> + unsigned int clk_type;
>> bool revoked;
>> unsigned int bufsize;
>> struct input_event buffer[];
>> @@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client
>> *client)
>> static int evdev_set_clk_type(struct evdev_client *client, unsigned int
>> clkid)
>> {
>> unsigned long flags;
>> -
>> - if (client->clk_type == clkid)
>> - return 0;
>> + unsigned int prev_clk_type = client->clk_type;
>
> I prefer the other way around: convert to a temp and then compare. I;ll
> fix it up on my side.
>
I thought that was the only better way of all.
I am not sure what exactly you're referring. I guess I need little
more elaboration here or anyways, I'll check it up up once you apply
it.
>>
>> switch (clkid) {
>>
>> @@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client
>> *client, unsigned int clkid)
>> return -EINVAL;
>> }
>>
>> + /* No need to flush if clk_type is same as before */
>> + if (client->clk_type == prev_clk_type)
>> + return 0;
>> +
>> /*
>>* Flush pending events and queue SYN_DROPPED event,
>>* but only if the queue is not empty.
>> --
>> 2.6.2
>>
>
> Thanks.
>
> --
> Dmitry
Regards,
Aniroop
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Evdev: Fix bug in checking duplicate clock change request
From: Aniroop Mathur
clk_type and clkid stores different predefined clock identification
values so they cannot be compared for checking duplicate clock change
request. Therefore, lets fix it to avoid unexpected results.
Signed-off-by: Aniroop Mathur
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 08d4964..0d40f6f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -56,7 +56,7 @@ struct evdev_client {
struct fasync_struct *fasync;
struct evdev *evdev;
struct list_head node;
- int clk_type;
+ unsigned int clk_type;
bool revoked;
unsigned int bufsize;
struct input_event buffer[];
@@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client
*client)
static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
{
unsigned long flags;
-
- if (client->clk_type == clkid)
- return 0;
+ unsigned int prev_clk_type = client->clk_type;
switch (clkid) {
@@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
return -EINVAL;
}
+ /* No need to flush if clk_type is same as before */
+ if (client->clk_type == prev_clk_type)
+ return 0;
+
/*
* Flush pending events and queue SYN_DROPPED event,
* but only if the queue is not empty.
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Evdev: Fix bug of comparing clk_type with clkid
From: Aniroop Mathur
clk_type and clkid stores different predefined clock identification
values so they cannot be compared.
Therefore, lets fix it to avoid unexpected results.
Signed-off-by: Aniroop Mathur
Signed-off-by: Aniroop Mathur
---
drivers/input/evdev.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 08d4964..0d40f6f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -56,7 +56,7 @@ struct evdev_client {
struct fasync_struct *fasync;
struct evdev *evdev;
struct list_head node;
- int clk_type;
+ unsigned int clk_type;
bool revoked;
unsigned int bufsize;
struct input_event buffer[];
@@ -146,9 +146,7 @@ static void evdev_queue_syn_dropped(struct evdev_client
*client)
static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
{
unsigned long flags;
-
- if (client->clk_type == clkid)
- return 0;
+ unsigned int prev_clk_type = client->clk_type;
switch (clkid) {
@@ -165,6 +163,10 @@ static int evdev_set_clk_type(struct evdev_client *client,
unsigned int clkid)
return -EINVAL;
}
+ /* No need to flush if clk_type is same as before */
+ if (client->clk_type == prev_clk_type)
+ return 0;
+
/*
* Flush pending events and queue SYN_DROPPED event,
* but only if the queue is not empty.
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] hid: Initialize battery_no to -1 & correct its format string
Hello Mr. Benjamin,
On Wed, Jan 7, 2015 at 7:20 AM, Benjamin Tissoires
wrote:
> On Tue, Jan 6, 2015 at 7:50 PM, Ping Cheng wrote:
>> Hi Benjamin,
>>
>> The relevant code was introduced by
>> d70420b914c98a3758674c6e9858810e0ab4ea30. Can you take a look and let
>> us know if Aniroop's patch fits your original thought or not?
>
> Hehe, indeed, that's my code.
>
> I would agree with Jiri, I really do not care. This code is called
> only once at initialization and I am pretty sure it will not change
> the time to initialize the device (I am confident the compiler will do
> something better than blindly following the code).
>
> So either way it is good. But I have a slight preference to initialize
> the variable to 0 and keep the unsigned in the name.
>
> Cheers,
> Benjamin
>
Atleast, there is a need to fix format specifier of "n" variable.
"n" is declared as unsigned long so %ld should be changed to %lu.
Thanks !!
>>
>> Thanks,
>>
>> Ping
>>
>> On Tue, Jan 6, 2015 at 6:32 AM, Aniroop Mathur
>> wrote:
>>> Dear Mr. Jason and Mr. Ping,
>>>
>>> Please update about below patch.
>>> Except avoiding subtraction, there is also a need to avoid negative
>>> battery name which may come due to %ld, so need to change to %lu.
>>>
>>> Thanks,
>>> Aniroop
>>>
>>> On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina wrote:
>>>> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>>>>
>>>>> From: Aniroop Mathur
>>>>>
>>>>> This patch initializes battery_no to -1 to avoid extra subtraction
>>>>> operation performed everytime wacom battery is initialized
>>>>
>>>> This is so femto-optimization, that I don't really care deeply. Adding
>>>> Jason and Ping to CC. If they want this, I'll apply the patch.
>>>>
>>>>> and correct format string of unsigned long from %ld to %lu.
>>>>>
>>>>> Signed-off-by: Aniroop Mathur
>>>>> ---
>>>>> drivers/hid/wacom_sys.c | 8
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>>>> index 129fd33..4b5ff84 100644
>>>>> --- a/drivers/hid/wacom_sys.c
>>>>> +++ b/drivers/hid/wacom_sys.c
>>>>> @@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct
>>>>> power_supply *psy,
>>>>>
>>>>> static int wacom_initialize_battery(struct wacom *wacom)
>>>>> {
>>>>> - static atomic_t battery_no = ATOMIC_INIT(0);
>>>>> + static atomic_t battery_no = ATOMIC_INIT(-1);
>>>>> int error;
>>>>> unsigned long n;
>>>>>
>>>>> if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
>>>>> - n = atomic_inc_return(&battery_no) - 1;
>>>>> + n = atomic_inc_return(&battery_no);
>>>>>
>>>>> wacom->battery.properties = wacom_battery_props;
>>>>> wacom->battery.num_properties =
>>>>> ARRAY_SIZE(wacom_battery_props);
>>>>> wacom->battery.get_property = wacom_battery_get_property;
>>>>> - sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
>>>>> + sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
>>>>> wacom->battery.name = wacom->wacom_wac.bat_name;
>>>>> wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
>>>>> wacom->battery.use_for_apm = 0;
>>>>> @@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom
>>>>> *wacom)
>>>>> wacom->ac.properties = wacom_ac_props;
>>>>> wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
>>>>> wacom->ac.get_property = wacom_ac_get_property;
>>>>> - sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
>>>>> + sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
>>>>> wacom->ac.name = wacom->wacom_wac.ac_name;
>>>>> wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
>>>>> wacom->ac.use_for_apm = 0;
>>>>> --
>>>>> 1.9.1
>>>>>
>>>>
>>>> --
>>>> Jiri Kosina
>>>> SUSE Labs
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] hid: Initialize battery_no to -1 & correct its format string
Dear Mr. Jason and Mr. Ping,
Please update about below patch.
Except avoiding subtraction, there is also a need to avoid negative
battery name which may come due to %ld, so need to change to %lu.
Thanks,
Aniroop
On Mon, Dec 29, 2014 at 5:33 PM, Jiri Kosina wrote:
> On Sun, 28 Dec 2014, Aniroop Mathur wrote:
>
>> From: Aniroop Mathur
>>
>> This patch initializes battery_no to -1 to avoid extra subtraction
>> operation performed everytime wacom battery is initialized
>
> This is so femto-optimization, that I don't really care deeply. Adding
> Jason and Ping to CC. If they want this, I'll apply the patch.
>
>> and correct format string of unsigned long from %ld to %lu.
>>
>> Signed-off-by: Aniroop Mathur
>> ---
>> drivers/hid/wacom_sys.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>> index 129fd33..4b5ff84 100644
>> --- a/drivers/hid/wacom_sys.c
>> +++ b/drivers/hid/wacom_sys.c
>> @@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply
>> *psy,
>>
>> static int wacom_initialize_battery(struct wacom *wacom)
>> {
>> - static atomic_t battery_no = ATOMIC_INIT(0);
>> + static atomic_t battery_no = ATOMIC_INIT(-1);
>> int error;
>> unsigned long n;
>>
>> if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
>> - n = atomic_inc_return(&battery_no) - 1;
>> + n = atomic_inc_return(&battery_no);
>>
>> wacom->battery.properties = wacom_battery_props;
>> wacom->battery.num_properties =
>> ARRAY_SIZE(wacom_battery_props);
>> wacom->battery.get_property = wacom_battery_get_property;
>> - sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
>> + sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
>> wacom->battery.name = wacom->wacom_wac.bat_name;
>> wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
>> wacom->battery.use_for_apm = 0;
>> @@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
>> wacom->ac.properties = wacom_ac_props;
>> wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
>> wacom->ac.get_property = wacom_ac_get_property;
>> - sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
>> + sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
>> wacom->ac.name = wacom->wacom_wac.ac_name;
>> wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
>> wacom->ac.use_for_apm = 0;
>> --
>> 1.9.1
>>
>
> --
> Jiri Kosina
> SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] regulator: core: Fix format specifier warning
On Mon, Dec 29, 2014 at 10:40 PM, Mark Brown wrote: > On Mon, Dec 29, 2014 at 10:36:48PM +0530, Aniroop Mathur wrote: >> Signed-off-by: Aniroop Mathur > > Applied, thanks. Please check what you're generating your patches > against - they aren't applying cleanly, you should be using current > development code. Sure, I will take care of it from next time. Thanks !! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regulator: core: Fix format specifier warning
Signed-off-by: Aniroop Mathur
---
drivers/regulator/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index b0729ef..f380f04 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3551,7 +3551,7 @@ regulator_register(const struct regulator_desc
*regulator_desc,
rdev->dev.of_node = of_node_get(config->of_node);
rdev->dev.parent = dev;
dev_set_name(&rdev->dev, "regulator.%lu",
-atomic_inc_return(®ulator_no));
+ (unsigned long) atomic_inc_return(®ulator_no));
ret = device_register(&rdev->dev);
if (ret != 0) {
put_device(&rdev->dev);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regulator: core: Avoid negative regulator no & initialize it to -1
This patch initializes regulator_no to -1 to avoid extra subtraction
operation performed every time we register a regulator and avoid negative
regulator no in its name.
Signed-off-by: Aniroop Mathur
---
drivers/regulator/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a3c3785..b0729ef 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3481,7 +3481,7 @@ regulator_register(const struct regulator_desc
*regulator_desc,
{
const struct regulation_constraints *constraints = NULL;
const struct regulator_init_data *init_data;
- static atomic_t regulator_no = ATOMIC_INIT(0);
+ static atomic_t regulator_no = ATOMIC_INIT(-1);
struct regulator_dev *rdev;
struct device *dev;
int ret, i;
@@ -3550,8 +3550,8 @@ regulator_register(const struct regulator_desc
*regulator_desc,
rdev->dev.class = ®ulator_class;
rdev->dev.of_node = of_node_get(config->of_node);
rdev->dev.parent = dev;
- dev_set_name(&rdev->dev, "regulator.%d",
-atomic_inc_return(®ulator_no) - 1);
+ dev_set_name(&rdev->dev, "regulator.%lu",
+atomic_inc_return(®ulator_no));
ret = device_register(&rdev->dev);
if (ret != 0) {
put_device(&rdev->dev);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] s390: Initialize nr_devices count variable to -1
From: Aniroop Mathur
This patch initializes nr_device count variable to -1 to avoid extra
subtraction operation performed everytime scm block device is set up.
Signed-off-by: Aniroop Mathur
---
drivers/s390/block/scm_blk.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/s390/block/scm_blk.c b/drivers/s390/block/scm_blk.c
index 76bed17..ee2bd6f 100644
--- a/drivers/s390/block/scm_blk.c
+++ b/drivers/s390/block/scm_blk.c
@@ -23,7 +23,7 @@ static int scm_major;
static DEFINE_SPINLOCK(list_lock);
static LIST_HEAD(inactive_requests);
static unsigned int nr_requests = 64;
-static atomic_t nr_devices = ATOMIC_INIT(0);
+static atomic_t nr_devices = ATOMIC_INIT(-1);
module_param(nr_requests, uint, S_IRUGO);
MODULE_PARM_DESC(nr_requests, "Number of parallel requests.");
@@ -357,7 +357,7 @@ int scm_blk_dev_setup(struct scm_blk_dev *bdev, struct
scm_device *scmdev)
int len, ret = -ENOMEM;
unsigned int devindex, nr_max_blk;
- devindex = atomic_inc_return(&nr_devices) - 1;
+ devindex = atomic_inc_return(&nr_devices);
/* scma..scmz + scmaa..scmzz */
if (devindex > 701) {
ret = -ENODEV;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] hid: Initialize battery_no to -1 & correct its format string
From: Aniroop Mathur
This patch initializes battery_no to -1 to avoid extra subtraction
operation performed everytime wacom battery is initialized and
correct format string of unsigned long from %ld to %lu.
Signed-off-by: Aniroop Mathur
---
drivers/hid/wacom_sys.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 129fd33..4b5ff84 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -919,17 +919,17 @@ static int wacom_ac_get_property(struct power_supply *psy,
static int wacom_initialize_battery(struct wacom *wacom)
{
- static atomic_t battery_no = ATOMIC_INIT(0);
+ static atomic_t battery_no = ATOMIC_INIT(-1);
int error;
unsigned long n;
if (wacom->wacom_wac.features.quirks & WACOM_QUIRK_BATTERY) {
- n = atomic_inc_return(&battery_no) - 1;
+ n = atomic_inc_return(&battery_no);
wacom->battery.properties = wacom_battery_props;
wacom->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
wacom->battery.get_property = wacom_battery_get_property;
- sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%ld", n);
+ sprintf(wacom->wacom_wac.bat_name, "wacom_battery_%lu", n);
wacom->battery.name = wacom->wacom_wac.bat_name;
wacom->battery.type = POWER_SUPPLY_TYPE_BATTERY;
wacom->battery.use_for_apm = 0;
@@ -937,7 +937,7 @@ static int wacom_initialize_battery(struct wacom *wacom)
wacom->ac.properties = wacom_ac_props;
wacom->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
wacom->ac.get_property = wacom_ac_get_property;
- sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%ld", n);
+ sprintf(wacom->wacom_wac.ac_name, "wacom_ac_%lu", n);
wacom->ac.name = wacom->wacom_wac.ac_name;
wacom->ac.type = POWER_SUPPLY_TYPE_MAINS;
wacom->ac.use_for_apm = 0;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] regulator: core: Initialize regulator_no to -1
From: Aniroop Mathur
This patch initializes regulator_no to -1 to avoid extra subtraction
operation performed every time we register a regulator.
Signed-off-by: Aniroop Mathur
---
drivers/regulator/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index a3c3785..4da1d98 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3481,7 +3481,7 @@ regulator_register(const struct regulator_desc
*regulator_desc,
{
const struct regulation_constraints *constraints = NULL;
const struct regulator_init_data *init_data;
- static atomic_t regulator_no = ATOMIC_INIT(0);
+ static atomic_t regulator_no = ATOMIC_INIT(-1);
struct regulator_dev *rdev;
struct device *dev;
int ret, i;
@@ -3551,7 +3551,7 @@ regulator_register(const struct regulator_desc
*regulator_desc,
rdev->dev.of_node = of_node_get(config->of_node);
rdev->dev.parent = dev;
dev_set_name(&rdev->dev, "regulator.%d",
-atomic_inc_return(®ulator_no) - 1);
+atomic_inc_return(®ulator_no));
ret = device_register(&rdev->dev);
if (ret != 0) {
put_device(&rdev->dev);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] IIO: Added write function in iio_buffer_fileops
On Fri, Aug 22, 2014 at 11:58 PM, Jonathan Cameron wrote: > On 16/08/14 15:44, Aniroop Mathur wrote: >> >> >> >> On Thu, Aug 14, 2014 at 8:08 PM, Jonathan Cameron > <mailto:ji...@kernel.org>> wrote: >> >> On 14/08/14 10:41, Lars-Peter Clausen wrote: >> > On 08/13/2014 06:33 PM, Aniroop Mathur wrote: >> >> On Wed, Aug 13, 2014 at 8:18 PM, Lars-Peter Clausen > <mailto:l...@metafoo.de>> wrote: >> >>> On 08/13/2014 03:42 PM, Aniroop Mathur wrote: >> >>>> >> >>>> On Wed, Aug 13, 2014 at 2:38 PM, Lars-Peter Clausen >> mailto:l...@metafoo.de>> >> >>>> wrote: >> >>>>> >> >>>>> On 08/13/2014 08:29 AM, a.mat...@samsung.com >> <mailto:a.mat...@samsung.com> wrote: >> >>>>>> >> >>>>>> >> >>>>>> From: Aniroop Mathur > <mailto:a.mat...@samsung.com>> >> >>>>>> >> >>>>>> Earlier, user space can only read from iio device node but cannot >> write >> >>>>>> to >> >>>>>> it. >> >>>>>> This patch adds write function in iio buffer file operations, >> >>>>>> which will allow user-space applications/HAL to write the data >> >>>>>> to iio device node. >> >>>>>> So now there will be two way communication between IIO subsystem >> >>>>>> and user space. (userspace <--> kernel) >> >>>>>> >> >>>>>> It can be used by HAL or any user-space application which wants to >> >>>>>> write data to iio device node/buffer upon receiving some data >> from it. >> >>>>>> As an example, >> >>>>>> It is useful for iio device simulator application which need to >> record >> >>>>>> the data by reading from iio device node and replay the recorded >> data >> >>>>>> by writing back to iio device node. >> >>>>>> >> >>>>> >> >>>>> I'm not convinced that this is something that should be added to >> the >> >>>>> kernel. >> I'm inclined to agree with Lars. As an additional point this will cause >> confusion when we have buffered writing for output devices (DACs). >> >> >> >>>>> I'm wondering why can't this be done in userspace, e.g. by having a >> >>>>> simulator mode for the application or by using LD_PRELOAD. Having >> this in >> >>>>> userspace will be much more flexible and will be easier to >> implement >> >>>>> correctly and you'll most likely want to simulate more than just >> buffer >> >>>>> access, for example setting/getting properties of the device or >> channel. >> >>>>> For >> >>>>> the libiio[1] we are planning to implement a test backend that when >> >>>>> activated will allow to simulate a whole device rather than just >> buffer >> >>>>> support. >> >>>>> >> >>>>> [1] https://github.com/analogdevicesinc/libiio >> >>>>> >> >>>>> >> >>>> >> >>>> In normal Input Subsystem, there is two way communication between >> >>>> kernel and userpace. It has both read and write functionality. :) >> >>>> Why there is only one way communication in case of IIO ? >> Why should there be? >> >> >> >> Below is the use case for which write function is required: >> >> I am working on Sensor Simulator application for android phones. > A worthy goal! >> This android application will simulate the sensor values >> for any other already developed 3rd party/organization application/s >> and not for simulator application itself. >> In other words, I need to check the working of other sensor applications by >> using my simulator application and along with it, checking of HAL and >> Framework too. >> >> For simulation, below is the data flow >> by using user-space libiio and by directly writing to iio buffer. >> >> Hardware--Driver--IIO--Gen
Re: [PATCH 1/1] IIO: Added write function in iio_buffer_fileops
Dear Mr. Jonathan, Mr. Lars, Greetings ! Kindly provide feedback upon the comments below. On Thu, Aug 14, 2014 at 8:08 PM, Jonathan Cameron wrote: > On 14/08/14 10:41, Lars-Peter Clausen wrote: >> On 08/13/2014 06:33 PM, Aniroop Mathur wrote: >>> On Wed, Aug 13, 2014 at 8:18 PM, Lars-Peter Clausen wrote: >>>> On 08/13/2014 03:42 PM, Aniroop Mathur wrote: >>>>> >>>>> On Wed, Aug 13, 2014 at 2:38 PM, Lars-Peter Clausen >>>>> wrote: >>>>>> >>>>>> On 08/13/2014 08:29 AM, a.mat...@samsung.com wrote: >>>>>>> >>>>>>> >>>>>>> From: Aniroop Mathur >>>>>>> >>>>>>> Earlier, user space can only read from iio device node but cannot write >>>>>>> to >>>>>>> it. >>>>>>> This patch adds write function in iio buffer file operations, >>>>>>> which will allow user-space applications/HAL to write the data >>>>>>> to iio device node. >>>>>>> So now there will be two way communication between IIO subsystem >>>>>>> and user space. (userspace <--> kernel) >>>>>>> >>>>>>> It can be used by HAL or any user-space application which wants to >>>>>>> write data to iio device node/buffer upon receiving some data from it. >>>>>>> As an example, >>>>>>> It is useful for iio device simulator application which need to record >>>>>>> the data by reading from iio device node and replay the recorded data >>>>>>> by writing back to iio device node. >>>>>>> >>>>>> >>>>>> I'm not convinced that this is something that should be added to the >>>>>> kernel. > I'm inclined to agree with Lars. As an additional point this will cause > confusion when we have buffered writing for output devices (DACs). > > >>>>>> I'm wondering why can't this be done in userspace, e.g. by having a >>>>>> simulator mode for the application or by using LD_PRELOAD. Having this in >>>>>> userspace will be much more flexible and will be easier to implement >>>>>> correctly and you'll most likely want to simulate more than just buffer >>>>>> access, for example setting/getting properties of the device or channel. >>>>>> For >>>>>> the libiio[1] we are planning to implement a test backend that when >>>>>> activated will allow to simulate a whole device rather than just buffer >>>>>> support. >>>>>> >>>>>> [1] https://github.com/analogdevicesinc/libiio >>>>>> >>>>>> >>>>> >>>>> In normal Input Subsystem, there is two way communication between >>>>> kernel and userpace. It has both read and write functionality. :) >>>>> Why there is only one way communication in case of IIO ? > Why should there be? Below is the use case for which write function is required: I am working on Sensor Simulator application for android phones. This android application will simulate the sensor values for any other already developed 3rd party/organization application/s and not for simulator application itself. In other words, I need to check the working of other sensor applications by using my simulator application and along with it, checking of HAL and Framework too. For simulation, below is the data flow by using user-space libiio and by directly writing to iio buffer. Hardware--Driver--IIO--General_IIO_HAL--Framework--Other_Sensor_Application/s | | --> libiio <--> Simulator_Sensor_Application | | <--> Simulator_Sensor_Application If we use libiio, Sensor_Simulator_Application will not be able to send sensor data to Other_Sensor_Application/s because it can only write and read data for itself. Reading and writing data for itself is not what is required. There is a need to send data to other application/s. Also, we could not check HAL and Framework here. If we directly write to iio buffer, Sensor_Simulator_Application will be able to send sensor data to Other_Sensor_Application/s and this in turn will also check whether there are any problems in hal and framework for Other_Application to work properly. So, if some Other_Sensor_Application is behaving differently for same sensor data it means there is some probl
Re: [PATCH 1/1] IIO: Added write function in iio_buffer_fileops
On Thu, Aug 14, 2014 at 8:08 PM, Jonathan Cameron wrote: > On 14/08/14 10:41, Lars-Peter Clausen wrote: >> On 08/13/2014 06:33 PM, Aniroop Mathur wrote: >>> On Wed, Aug 13, 2014 at 8:18 PM, Lars-Peter Clausen wrote: >>>> On 08/13/2014 03:42 PM, Aniroop Mathur wrote: >>>>> >>>>> On Wed, Aug 13, 2014 at 2:38 PM, Lars-Peter Clausen >>>>> wrote: >>>>>> >>>>>> On 08/13/2014 08:29 AM, a.mat...@samsung.com wrote: >>>>>>> >>>>>>> >>>>>>> From: Aniroop Mathur >>>>>>> >>>>>>> Earlier, user space can only read from iio device node but cannot write >>>>>>> to >>>>>>> it. >>>>>>> This patch adds write function in iio buffer file operations, >>>>>>> which will allow user-space applications/HAL to write the data >>>>>>> to iio device node. >>>>>>> So now there will be two way communication between IIO subsystem >>>>>>> and user space. (userspace <--> kernel) >>>>>>> >>>>>>> It can be used by HAL or any user-space application which wants to >>>>>>> write data to iio device node/buffer upon receiving some data from it. >>>>>>> As an example, >>>>>>> It is useful for iio device simulator application which need to record >>>>>>> the data by reading from iio device node and replay the recorded data >>>>>>> by writing back to iio device node. >>>>>>> >>>>>> >>>>>> I'm not convinced that this is something that should be added to the >>>>>> kernel. > I'm inclined to agree with Lars. As an additional point this will cause > confusion when we have buffered writing for output devices (DACs). > > >>>>>> I'm wondering why can't this be done in userspace, e.g. by having a >>>>>> simulator mode for the application or by using LD_PRELOAD. Having this in >>>>>> userspace will be much more flexible and will be easier to implement >>>>>> correctly and you'll most likely want to simulate more than just buffer >>>>>> access, for example setting/getting properties of the device or channel. >>>>>> For >>>>>> the libiio[1] we are planning to implement a test backend that when >>>>>> activated will allow to simulate a whole device rather than just buffer >>>>>> support. >>>>>> >>>>>> [1] https://github.com/analogdevicesinc/libiio >>>>>> >>>>>> >>>>> >>>>> In normal Input Subsystem, there is two way communication between >>>>> kernel and userpace. It has both read and write functionality. :) >>>>> Why there is only one way communication in case of IIO ? > Why should there be? Below is my use case for which write function is required: I am working on Sensor Simulator application for android phones. This android application will simulate the sensor values for any other already developed 3rd party/organization application/s and not for simulator application itself. In other words, I need to check the working of other sensor applications by using my simulator application and along with it, checking of HAL and Framework too. For simulation, below is the data flow by using user-space libiio and by directly writing to iio buffer. Hardware--Driver--IIO--General_IIO_HAL--Framework--Other_Sensor_Application/s | | --> libiio <--> Simulator_Sensor_Application | | <--> Simulator_Sensor_Application If we use libiio, Sensor_Simulator_Application will not be able to send sensor data to Other_Sensor_Application/s because it can only write and read data for itself. Reading and writing data for itself is not what is required. There is a need to send data to other application/s. Also, we could not check HAL and Framework here. If we directly write to iio buffer, Sensor_Simulator_Application will be able to send sensor data to Other_Sensor_Application/s and this in turn will also check whether there are any problems in hal and framework for Other_Application to work properly. So, if some Other_Sensor_Application is behaving differently for same sensor data it means there is some problem in HAL/Framework/Application itself. Also importantly, we can change the recorded data and check Other_Application working with that new da
