Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Wed, Jun 28, 2017 at 06:30:44AM +0200, Michał Kępień wrote:
> > On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > > On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > > > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > > > which causes the supplied callback to be queued to a static workqueue
> > > > > which always executes on CPU 0.  This means that there is no 
> > > > > possibility
> > > > > for any ACPI device notify callback to be concurrently executed on
> > > > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes 
> > > > > are
> > > > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no 
> > > > > risk
> > > > > of concurrent pushing and popping exists.
> > > > 
> > > > Was the kfifo causing a problem currently or for the migration to 
> > > > separate
> > > > modules? Is this purely a simplification?
> > > > 
> > > > Rafael, the above rationale appears sound to me. Do you have any 
> > > > concerns?
> > > 
> > > I actually do.
> > > 
> > > While this is the case today, making the driver code depend on it in a 
> > > hard way
> > > sort of makes it difficult to change in the future if need be.
> > 
> > OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
> > will be annoying to debug if it does changes, let's skip the kfifo change.
> > 
> > I have removed this patch, and fixed up the merge conflicts of the 
> > remaining 6
> > patches here:
> > 
> > http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> > 
> > Michal / Jonathan, would you please review and let me know if this is what 
> > you
> > would have done / approve the rebase?
> 
> The only issue I can see is that the push/pop debug messages in the last
> patch contain the word "buffer" instead of the original "ringbuffer".
> The dropped kfifo patch changed the wording from "ringbuffer" to
> "buffer" as applying it means there is no ringbuffer any more, but since
> it was not applied in the end, I guess the original wording should stay
> in place.
> 
> The rest looks good to me.

Good catch, thank you. The testing branch has been rebased to reflect these
changes.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Wed, Jun 28, 2017 at 06:30:44AM +0200, Michał Kępień wrote:
> > On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > > On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > > > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > > > which causes the supplied callback to be queued to a static workqueue
> > > > > which always executes on CPU 0.  This means that there is no 
> > > > > possibility
> > > > > for any ACPI device notify callback to be concurrently executed on
> > > > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes 
> > > > > are
> > > > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no 
> > > > > risk
> > > > > of concurrent pushing and popping exists.
> > > > 
> > > > Was the kfifo causing a problem currently or for the migration to 
> > > > separate
> > > > modules? Is this purely a simplification?
> > > > 
> > > > Rafael, the above rationale appears sound to me. Do you have any 
> > > > concerns?
> > > 
> > > I actually do.
> > > 
> > > While this is the case today, making the driver code depend on it in a 
> > > hard way
> > > sort of makes it difficult to change in the future if need be.
> > 
> > OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
> > will be annoying to debug if it does changes, let's skip the kfifo change.
> > 
> > I have removed this patch, and fixed up the merge conflicts of the 
> > remaining 6
> > patches here:
> > 
> > http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> > 
> > Michal / Jonathan, would you please review and let me know if this is what 
> > you
> > would have done / approve the rebase?
> 
> The only issue I can see is that the push/pop debug messages in the last
> patch contain the word "buffer" instead of the original "ringbuffer".
> The dropped kfifo patch changed the wording from "ringbuffer" to
> "buffer" as applying it means there is no ringbuffer any more, but since
> it was not applied in the end, I guess the original wording should stay
> in place.
> 
> The rest looks good to me.

Good catch, thank you. The testing branch has been rebased to reflect these
changes.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

> On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > > which causes the supplied callback to be queued to a static workqueue
> > > > which always executes on CPU 0.  This means that there is no possibility
> > > > for any ACPI device notify callback to be concurrently executed on
> > > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > > of concurrent pushing and popping exists.
> > > 
> > > Was the kfifo causing a problem currently or for the migration to separate
> > > modules? Is this purely a simplification?
> > > 
> > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> > 
> > I actually do.
> > 
> > While this is the case today, making the driver code depend on it in a hard 
> > way
> > sort of makes it difficult to change in the future if need be.
> 
> OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
> will be annoying to debug if it does changes, let's skip the kfifo change.
> 
> I have removed this patch, and fixed up the merge conflicts of the remaining 6
> patches here:
> 
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> 
> Michal / Jonathan, would you please review and let me know if this is what you
> would have done / approve the rebase?

The only issue I can see is that the push/pop debug messages in the last
patch contain the word "buffer" instead of the original "ringbuffer".
The dropped kfifo patch changed the wording from "ringbuffer" to
"buffer" as applying it means there is no ringbuffer any more, but since
it was not applied in the end, I guess the original wording should stay
in place.

The rest looks good to me.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

> On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > > which causes the supplied callback to be queued to a static workqueue
> > > > which always executes on CPU 0.  This means that there is no possibility
> > > > for any ACPI device notify callback to be concurrently executed on
> > > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > > of concurrent pushing and popping exists.
> > > 
> > > Was the kfifo causing a problem currently or for the migration to separate
> > > modules? Is this purely a simplification?
> > > 
> > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> > 
> > I actually do.
> > 
> > While this is the case today, making the driver code depend on it in a hard 
> > way
> > sort of makes it difficult to change in the future if need be.
> 
> OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
> will be annoying to debug if it does changes, let's skip the kfifo change.
> 
> I have removed this patch, and fixed up the merge conflicts of the remaining 6
> patches here:
> 
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> 
> Michal / Jonathan, would you please review and let me know if this is what you
> would have done / approve the rebase?

The only issue I can see is that the push/pop debug messages in the last
patch contain the word "buffer" instead of the original "ringbuffer".
The dropped kfifo patch changed the wording from "ringbuffer" to
"buffer" as applying it means there is no ringbuffer any more, but since
it was not applied in the end, I guess the original wording should stay
in place.

The rest looks good to me.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

On Mon, Jun 26, 2017 at 05:07:18PM -0700, Darren Hart wrote:
> On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> > 
> > I actually do.
> > 
> > While this is the case today, making the driver code depend on it in a hard 
> > way
> > sort of makes it difficult to change in the future if need be.
> 
> OK, if we aren't guaranteed for this to run on CPU 0 in the future, and
> this will be annoying to debug if it does changes, let's skip the kfifo
> change.
> 
> I have removed this patch, and fixed up the merge conflicts of the
> remaining 6 patches here:
> 
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> 
> Michal / Jonathan, would you please review and let me know if this is what
> you would have done / approve the rebase?

The rebase looks reasonable to me.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

On Mon, Jun 26, 2017 at 05:07:18PM -0700, Darren Hart wrote:
> On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> > > Rafael, the above rationale appears sound to me. Do you have any concerns?
> > 
> > I actually do.
> > 
> > While this is the case today, making the driver code depend on it in a hard 
> > way
> > sort of makes it difficult to change in the future if need be.
> 
> OK, if we aren't guaranteed for this to run on CPU 0 in the future, and
> this will be annoying to debug if it does changes, let's skip the kfifo
> change.
> 
> I have removed this patch, and fixed up the merge conflicts of the
> remaining 6 patches here:
> 
> http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu
> 
> Michal / Jonathan, would you please review and let me know if this is what
> you would have done / approve the rebase?

The rebase looks reasonable to me.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > which causes the supplied callback to be queued to a static workqueue
> > > which always executes on CPU 0.  This means that there is no possibility
> > > for any ACPI device notify callback to be concurrently executed on
> > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > of concurrent pushing and popping exists.
> > 
> > Was the kfifo causing a problem currently or for the migration to separate
> > modules? Is this purely a simplification?
> > 
> > Rafael, the above rationale appears sound to me. Do you have any concerns?
> 
> I actually do.
> 
> While this is the case today, making the driver code depend on it in a hard 
> way
> sort of makes it difficult to change in the future if need be.

OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
will be annoying to debug if it does changes, let's skip the kfifo change.

I have removed this patch, and fixed up the merge conflicts of the remaining 6
patches here:

http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu

Michal / Jonathan, would you please review and let me know if this is what you
would have done / approve the rebase?

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Sat, Jun 24, 2017 at 02:25:46AM +0200, Rafael Wysocki wrote:
> On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > > which causes the supplied callback to be queued to a static workqueue
> > > which always executes on CPU 0.  This means that there is no possibility
> > > for any ACPI device notify callback to be concurrently executed on
> > > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > > of concurrent pushing and popping exists.
> > 
> > Was the kfifo causing a problem currently or for the migration to separate
> > modules? Is this purely a simplification?
> > 
> > Rafael, the above rationale appears sound to me. Do you have any concerns?
> 
> I actually do.
> 
> While this is the case today, making the driver code depend on it in a hard 
> way
> sort of makes it difficult to change in the future if need be.

OK, if we aren't guaranteed for this to run on CPU 0 in the future, and this
will be annoying to debug if it does changes, let's skip the kfifo change.

I have removed this patch, and fixed up the merge conflicts of the remaining 6
patches here:

http://git.infradead.org/linux-platform-drivers-x86.git/shortlog/refs/heads/fujitsu

Michal / Jonathan, would you please review and let me know if this is what you
would have done / approve the rebase?

Thanks,

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Rafael J. Wysocki]

On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > which causes the supplied callback to be queued to a static workqueue
> > which always executes on CPU 0.  This means that there is no possibility
> > for any ACPI device notify callback to be concurrently executed on
> > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > of concurrent pushing and popping exists.
> 
> Was the kfifo causing a problem currently or for the migration to separate
> modules? Is this purely a simplification?
> 
> Rafael, the above rationale appears sound to me. Do you have any concerns?

I actually do.

While this is the case today, making the driver code depend on it in a hard way
sort of makes it difficult to change in the future if need be.

Thanks,
Rafael

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Rafael J. Wysocki]

On Wednesday, June 21, 2017 11:15:43 AM Darren Hart wrote:
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > which causes the supplied callback to be queued to a static workqueue
> > which always executes on CPU 0.  This means that there is no possibility
> > for any ACPI device notify callback to be concurrently executed on
> > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > of concurrent pushing and popping exists.
> 
> Was the kfifo causing a problem currently or for the migration to separate
> modules? Is this purely a simplification?
> 
> Rafael, the above rationale appears sound to me. Do you have any concerns?

I actually do.

While this is the case today, making the driver code depend on it in a hard way
sort of makes it difficult to change in the future if need be.

Thanks,
Rafael

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Fri, Jun 23, 2017 at 09:44:58AM +0930, Jonathan Woithe wrote:
> On Thu, Jun 22, 2017 at 04:58:09PM -0700, Darren Hart wrote:
> > On Thu, Jun 22, 2017 at 10:46:19PM +0200, Micha?? K??pie?? wrote:
> > > > The events seen by userspace with the original code would be "A-press",
> > > > "B-press", "A-release", "B-release".  With the revised code the order 
> > > > of the
> > > > release events would be reversed compared to the previous behaviour.  
> > > > That
> > > > is, unless I've misunderstood how sparse_keymap_report_event() works.
> > > 
> > > All you wrote above is correct and this patch does change the order of
> > > release events sent to userspace when multiple hotkeys are pressed
> > > simultaneously.  The question is: is it relevant for any practical
> > > scenario?
> > > 
> > > Anyway, I find this matter to be of secondary importance.  The primary
> > > objective of this patch is to get rid of the kfifo.  If anyone has
> > > strong feelings about the change in event ordering, I will be happy to
> > > revert to FIFO in v2.
> > 
> > This all looks reasonable to me, I don't see anything requiring a respin.
> 
> I agree it is of seconary importance.  To me, using LIFO release order is
> counter-intuitive, but it's the sort of question that if put to 100 people
> you'll get a 50/50 split of opinions.
> 
> Especially since the whole "multiple buttons held at once" scenario is
> rather unusual we can go with switching the order if others don't see a
> problem with the behavioural change.

Yes. If anyone notices the implementation difference, I'll be rather surprised.
If they do, we can convert back to FIFO as you say.
-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Fri, Jun 23, 2017 at 09:44:58AM +0930, Jonathan Woithe wrote:
> On Thu, Jun 22, 2017 at 04:58:09PM -0700, Darren Hart wrote:
> > On Thu, Jun 22, 2017 at 10:46:19PM +0200, Micha?? K??pie?? wrote:
> > > > The events seen by userspace with the original code would be "A-press",
> > > > "B-press", "A-release", "B-release".  With the revised code the order 
> > > > of the
> > > > release events would be reversed compared to the previous behaviour.  
> > > > That
> > > > is, unless I've misunderstood how sparse_keymap_report_event() works.
> > > 
> > > All you wrote above is correct and this patch does change the order of
> > > release events sent to userspace when multiple hotkeys are pressed
> > > simultaneously.  The question is: is it relevant for any practical
> > > scenario?
> > > 
> > > Anyway, I find this matter to be of secondary importance.  The primary
> > > objective of this patch is to get rid of the kfifo.  If anyone has
> > > strong feelings about the change in event ordering, I will be happy to
> > > revert to FIFO in v2.
> > 
> > This all looks reasonable to me, I don't see anything requiring a respin.
> 
> I agree it is of seconary importance.  To me, using LIFO release order is
> counter-intuitive, but it's the sort of question that if put to 100 people
> you'll get a 50/50 split of opinions.
> 
> Especially since the whole "multiple buttons held at once" scenario is
> rather unusual we can go with switching the order if others don't see a
> problem with the behavioural change.

Yes. If anyone notices the implementation difference, I'll be rather surprised.
If they do, we can convert back to FIFO as you say.
-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

On Thu, Jun 22, 2017 at 04:58:09PM -0700, Darren Hart wrote:
> On Thu, Jun 22, 2017 at 10:46:19PM +0200, Micha?? K??pie?? wrote:
> > > The events seen by userspace with the original code would be "A-press",
> > > "B-press", "A-release", "B-release".  With the revised code the order of 
> > > the
> > > release events would be reversed compared to the previous behaviour.  That
> > > is, unless I've misunderstood how sparse_keymap_report_event() works.
> > 
> > All you wrote above is correct and this patch does change the order of
> > release events sent to userspace when multiple hotkeys are pressed
> > simultaneously.  The question is: is it relevant for any practical
> > scenario?
> > 
> > Anyway, I find this matter to be of secondary importance.  The primary
> > objective of this patch is to get rid of the kfifo.  If anyone has
> > strong feelings about the change in event ordering, I will be happy to
> > revert to FIFO in v2.
> 
> This all looks reasonable to me, I don't see anything requiring a respin.

I agree it is of seconary importance.  To me, using LIFO release order is
counter-intuitive, but it's the sort of question that if put to 100 people
you'll get a 50/50 split of opinions.

Especially since the whole "multiple buttons held at once" scenario is
rather unusual we can go with switching the order if others don't see a
problem with the behavioural change.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

On Thu, Jun 22, 2017 at 04:58:09PM -0700, Darren Hart wrote:
> On Thu, Jun 22, 2017 at 10:46:19PM +0200, Micha?? K??pie?? wrote:
> > > The events seen by userspace with the original code would be "A-press",
> > > "B-press", "A-release", "B-release".  With the revised code the order of 
> > > the
> > > release events would be reversed compared to the previous behaviour.  That
> > > is, unless I've misunderstood how sparse_keymap_report_event() works.
> > 
> > All you wrote above is correct and this patch does change the order of
> > release events sent to userspace when multiple hotkeys are pressed
> > simultaneously.  The question is: is it relevant for any practical
> > scenario?
> > 
> > Anyway, I find this matter to be of secondary importance.  The primary
> > objective of this patch is to get rid of the kfifo.  If anyone has
> > strong feelings about the change in event ordering, I will be happy to
> > revert to FIFO in v2.
> 
> This all looks reasonable to me, I don't see anything requiring a respin.

I agree it is of seconary importance.  To me, using LIFO release order is
counter-intuitive, but it's the sort of question that if put to 100 people
you'll get a 50/50 split of opinions.

Especially since the whole "multiple buttons held at once" scenario is
rather unusual we can go with switching the order if others don't see a
problem with the behavioural change.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Thu, Jun 22, 2017 at 10:46:19PM +0200, Michał Kępień wrote:
> > The events seen by userspace with the original code would be "A-press",
> > "B-press", "A-release", "B-release".  With the revised code the order of the
> > release events would be reversed compared to the previous behaviour.  That
> > is, unless I've misunderstood how sparse_keymap_report_event() works.
> 
> All you wrote above is correct and this patch does change the order of
> release events sent to userspace when multiple hotkeys are pressed
> simultaneously.  The question is: is it relevant for any practical
> scenario?
> 
> Anyway, I find this matter to be of secondary importance.  The primary
> objective of this patch is to get rid of the kfifo.  If anyone has
> strong feelings about the change in event ordering, I will be happy to
> revert to FIFO in v2.

This all looks reasonable to me, I don't see anything requiring a respin.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Thu, Jun 22, 2017 at 10:46:19PM +0200, Michał Kępień wrote:
> > The events seen by userspace with the original code would be "A-press",
> > "B-press", "A-release", "B-release".  With the revised code the order of the
> > release events would be reversed compared to the previous behaviour.  That
> > is, unless I've misunderstood how sparse_keymap_report_event() works.
> 
> All you wrote above is correct and this patch does change the order of
> release events sent to userspace when multiple hotkeys are pressed
> simultaneously.  The question is: is it relevant for any practical
> scenario?
> 
> Anyway, I find this matter to be of secondary importance.  The primary
> objective of this patch is to get rid of the kfifo.  If anyone has
> strong feelings about the change in event ordering, I will be happy to
> revert to FIFO in v2.

This all looks reasonable to me, I don't see anything requiring a respin.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

> Hi Darren
> 
> On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > > I think the buffer size could probably be reduced a little without 
> > > impacting
> > > on functionality.  I suspect the value was chosen so as to be well above 
> > > the
> > > number of events which could be generated ahead of a button release (which
> > > effectively releases all buttons held at that stage).  The number of hot
> > > keys is limited (I don't think any model has more than 5), so reducing the
> > > buffer somewhat appears safe (perhaps to 32 if there were any advantages 
> > > to
> > > having the size be a power of two).  There seems little point doing this 
> > > in
> > > the name of memory saving since the amount saved is trivially small.
> > 
> > It's not a problem as far as I'm concerned. Plenty more lower hanging
> > fruit if people want to reduce memory footprint.
> 
> Indeed.  I'm happy to leave it at 40.
> 
> > > I've given some more thought to the following point (from the original 
> > > patch
> > > submission):
> > > > In order to simplify implementation, hotkey input device behavior is
> > > > slightly changed (from FIFO to LIFO).  The order of release events
> > > > generated by the input device should not matter from end user
> > > > perspective as upon releasing any hotkey the firmware only produces a
> > > > single event which means "all hotkeys were released".
> > > 
> > > This will effectively alter the order the button events are seen by
> > > userspace though, won't it?  It would mean that (for example) a user who
> > > presses (and holds) the "media" key before "email" will end up with 
> > > "email"
> > > being acted on first.  This may surprise them.  Then again, I suppose it
> > > could be argued that such usage is unusual and therefore is likely to be
> > > rare - and therefore not worth bothering about.
> > 
> > Michal noted there is only one event to release all keys so the order 
> > wouldn't
> > be noticed in userspace. Has this been confirmed with testing?

I may have worded this in a confusing way, sorry for that.  What I
wanted to convey is that the driver currently produces release events in
FIFO order, but that order is not imposed by the firmware.

> 
> I can't test this because my Fujitsu doesn't have these hotkeys.
> 
> Regarding the order, the original code sent "press" events to userspace in
> the order that the keys were pressed.  When the single "release all keys"
> event is received, a "release" event ws sent to userspace for each button
> which was pressed, in the order they were pressed.
> 
> If userspace acted on the button release event then the order of the
> synthesised "release" events could be significant to userspace, even if they
> are all generated at the same time in response to the first "release" event.
> 
> By way of example, let's say there are two hotkeys, "A" and "B".  Let us also
> say that the user does the following:
>  - Press and hold hotkey "A"
>  - Press and hold hotkey "B"
>  - Release one of these hotkeys
> 
> The events seen by userspace with the original code would be "A-press",
> "B-press", "A-release", "B-release".  With the revised code the order of the
> release events would be reversed compared to the previous behaviour.  That
> is, unless I've misunderstood how sparse_keymap_report_event() works.

All you wrote above is correct and this patch does change the order of
release events sent to userspace when multiple hotkeys are pressed
simultaneously.  The question is: is it relevant for any practical
scenario?

Anyway, I find this matter to be of secondary importance.  The primary
objective of this patch is to get rid of the kfifo.  If anyone has
strong feelings about the change in event ordering, I will be happy to
revert to FIFO in v2.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

> Hi Darren
> 
> On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > > I think the buffer size could probably be reduced a little without 
> > > impacting
> > > on functionality.  I suspect the value was chosen so as to be well above 
> > > the
> > > number of events which could be generated ahead of a button release (which
> > > effectively releases all buttons held at that stage).  The number of hot
> > > keys is limited (I don't think any model has more than 5), so reducing the
> > > buffer somewhat appears safe (perhaps to 32 if there were any advantages 
> > > to
> > > having the size be a power of two).  There seems little point doing this 
> > > in
> > > the name of memory saving since the amount saved is trivially small.
> > 
> > It's not a problem as far as I'm concerned. Plenty more lower hanging
> > fruit if people want to reduce memory footprint.
> 
> Indeed.  I'm happy to leave it at 40.
> 
> > > I've given some more thought to the following point (from the original 
> > > patch
> > > submission):
> > > > In order to simplify implementation, hotkey input device behavior is
> > > > slightly changed (from FIFO to LIFO).  The order of release events
> > > > generated by the input device should not matter from end user
> > > > perspective as upon releasing any hotkey the firmware only produces a
> > > > single event which means "all hotkeys were released".
> > > 
> > > This will effectively alter the order the button events are seen by
> > > userspace though, won't it?  It would mean that (for example) a user who
> > > presses (and holds) the "media" key before "email" will end up with 
> > > "email"
> > > being acted on first.  This may surprise them.  Then again, I suppose it
> > > could be argued that such usage is unusual and therefore is likely to be
> > > rare - and therefore not worth bothering about.
> > 
> > Michal noted there is only one event to release all keys so the order 
> > wouldn't
> > be noticed in userspace. Has this been confirmed with testing?

I may have worded this in a confusing way, sorry for that.  What I
wanted to convey is that the driver currently produces release events in
FIFO order, but that order is not imposed by the firmware.

> 
> I can't test this because my Fujitsu doesn't have these hotkeys.
> 
> Regarding the order, the original code sent "press" events to userspace in
> the order that the keys were pressed.  When the single "release all keys"
> event is received, a "release" event ws sent to userspace for each button
> which was pressed, in the order they were pressed.
> 
> If userspace acted on the button release event then the order of the
> synthesised "release" events could be significant to userspace, even if they
> are all generated at the same time in response to the first "release" event.
> 
> By way of example, let's say there are two hotkeys, "A" and "B".  Let us also
> say that the user does the following:
>  - Press and hold hotkey "A"
>  - Press and hold hotkey "B"
>  - Release one of these hotkeys
> 
> The events seen by userspace with the original code would be "A-press",
> "B-press", "A-release", "B-release".  With the revised code the order of the
> release events would be reversed compared to the previous behaviour.  That
> is, unless I've misunderstood how sparse_keymap_report_event() works.

All you wrote above is correct and this patch does change the order of
release events sent to userspace when multiple hotkeys are pressed
simultaneously.  The question is: is it relevant for any practical
scenario?

Anyway, I find this matter to be of secondary importance.  The primary
objective of this patch is to get rid of the kfifo.  If anyone has
strong feelings about the change in event ordering, I will be happy to
revert to FIFO in v2.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > which causes the supplied callback to be queued to a static workqueue
> > which always executes on CPU 0.  This means that there is no possibility
> > for any ACPI device notify callback to be concurrently executed on
> > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > of concurrent pushing and popping exists.
> 
> Was the kfifo causing a problem currently or for the migration to separate
> modules? Is this purely a simplification?

It is just another step in stripping fujitsu-laptop down to its bare
essentials.  If my reasoning quoted above is correct, using a locked
kfifo needlessly suggests potential concurrency issues, but it is
definitely not an error.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> > All ACPI device notify callbacks are invoked using acpi_os_execute(),
> > which causes the supplied callback to be queued to a static workqueue
> > which always executes on CPU 0.  This means that there is no possibility
> > for any ACPI device notify callback to be concurrently executed on
> > multiple CPUs, which in the case of fujitsu-laptop means that using a
> > locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> > only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> > of concurrent pushing and popping exists.
> 
> Was the kfifo causing a problem currently or for the migration to separate
> modules? Is this purely a simplification?

It is just another step in stripping fujitsu-laptop down to its bare
essentials.  If my reasoning quoted above is correct, using a locked
kfifo needlessly suggests potential concurrency issues, but it is
definitely not an error.

-- 
Best regards,
Michał Kępień

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

Hi Darren

On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > I think the buffer size could probably be reduced a little without impacting
> > on functionality.  I suspect the value was chosen so as to be well above the
> > number of events which could be generated ahead of a button release (which
> > effectively releases all buttons held at that stage).  The number of hot
> > keys is limited (I don't think any model has more than 5), so reducing the
> > buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> > having the size be a power of two).  There seems little point doing this in
> > the name of memory saving since the amount saved is trivially small.
> 
> It's not a problem as far as I'm concerned. Plenty more lower hanging
> fruit if people want to reduce memory footprint.

Indeed.  I'm happy to leave it at 40.

> > I've given some more thought to the following point (from the original patch
> > submission):
> > > In order to simplify implementation, hotkey input device behavior is
> > > slightly changed (from FIFO to LIFO).  The order of release events
> > > generated by the input device should not matter from end user
> > > perspective as upon releasing any hotkey the firmware only produces a
> > > single event which means "all hotkeys were released".
> > 
> > This will effectively alter the order the button events are seen by
> > userspace though, won't it?  It would mean that (for example) a user who
> > presses (and holds) the "media" key before "email" will end up with "email"
> > being acted on first.  This may surprise them.  Then again, I suppose it
> > could be argued that such usage is unusual and therefore is likely to be
> > rare - and therefore not worth bothering about.
> 
> Michal noted there is only one event to release all keys so the order wouldn't
> be noticed in userspace. Has this been confirmed with testing?

I can't test this because my Fujitsu doesn't have these hotkeys.

Regarding the order, the original code sent "press" events to userspace in
the order that the keys were pressed.  When the single "release all keys"
event is received, a "release" event ws sent to userspace for each button
which was pressed, in the order they were pressed.

If userspace acted on the button release event then the order of the
synthesised "release" events could be significant to userspace, even if they
are all generated at the same time in response to the first "release" event.

By way of example, let's say there are two hotkeys, "A" and "B".  Let us also
say that the user does the following:
 - Press and hold hotkey "A"
 - Press and hold hotkey "B"
 - Release one of these hotkeys

The events seen by userspace with the original code would be "A-press",
"B-press", "A-release", "B-release".  With the revised code the order of the
release events would be reversed compared to the previous behaviour.  That
is, unless I've misunderstood how sparse_keymap_report_event() works.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

Hi Darren

On Wed, Jun 21, 2017 at 07:44:13PM -0700, Darren Hart wrote:
> > I think the buffer size could probably be reduced a little without impacting
> > on functionality.  I suspect the value was chosen so as to be well above the
> > number of events which could be generated ahead of a button release (which
> > effectively releases all buttons held at that stage).  The number of hot
> > keys is limited (I don't think any model has more than 5), so reducing the
> > buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> > having the size be a power of two).  There seems little point doing this in
> > the name of memory saving since the amount saved is trivially small.
> 
> It's not a problem as far as I'm concerned. Plenty more lower hanging
> fruit if people want to reduce memory footprint.

Indeed.  I'm happy to leave it at 40.

> > I've given some more thought to the following point (from the original patch
> > submission):
> > > In order to simplify implementation, hotkey input device behavior is
> > > slightly changed (from FIFO to LIFO).  The order of release events
> > > generated by the input device should not matter from end user
> > > perspective as upon releasing any hotkey the firmware only produces a
> > > single event which means "all hotkeys were released".
> > 
> > This will effectively alter the order the button events are seen by
> > userspace though, won't it?  It would mean that (for example) a user who
> > presses (and holds) the "media" key before "email" will end up with "email"
> > being acted on first.  This may surprise them.  Then again, I suppose it
> > could be argued that such usage is unusual and therefore is likely to be
> > rare - and therefore not worth bothering about.
> 
> Michal noted there is only one event to release all keys so the order wouldn't
> be noticed in userspace. Has this been confirmed with testing?

I can't test this because my Fujitsu doesn't have these hotkeys.

Regarding the order, the original code sent "press" events to userspace in
the order that the keys were pressed.  When the single "release all keys"
event is received, a "release" event ws sent to userspace for each button
which was pressed, in the order they were pressed.

If userspace acted on the button release event then the order of the
synthesised "release" events could be significant to userspace, even if they
are all generated at the same time in response to the first "release" event.

By way of example, let's say there are two hotkeys, "A" and "B".  Let us also
say that the user does the following:
 - Press and hold hotkey "A"
 - Press and hold hotkey "B"
 - Release one of these hotkeys

The events seen by userspace with the original code would be "A-press",
"B-press", "A-release", "B-release".  With the revised code the order of the
release events would be reversed compared to the previous behaviour.  That
is, unless I've misunderstood how sparse_keymap_report_event() works.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Thu, Jun 22, 2017 at 09:20:21AM +0930, Jonathan Woithe wrote:
> On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > > -#define RINGBUFFERSIZE 40
> > >  
> > >  /* Debugging */
> > >  #define FUJLAPTOP_DBG_ERROR0x0001
> > > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > >   struct input_dev *input;
> > >   char phys[32];
> > >   struct platform_device *pf_device;
> > > - struct kfifo fifo;
> > > - spinlock_t fifo_lock;
> > > + int scancode_buf[40];
> > 
> > Do we know why 40 was used here? A single use magic number is fine, but it
> > would be good to document why it is what it is if we know.
> 
> I am unsure as to why 40 was chosen as the size.  The support for the
> hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
> which featured this functionality (mine doesn't).  I obviously can't speak
> for Peter but I suspect that he simply picked a number which seemed
> reasonable at the time.  I am unaware of any specific reason why the size
> was set at 40, and to be honest never throught to ask at the time the patch
> was proposed.
> 
> If we need a reason to justify the value of 40, I guess the best option is
> "because it seems reasonable".

That's not surprising, and it's fine. If we know, it's worth documenting. If
not, well, we do what we can :-)

> 
> I think the buffer size could probably be reduced a little without impacting
> on functionality.  I suspect the value was chosen so as to be well above the
> number of events which could be generated ahead of a button release (which
> effectively releases all buttons held at that stage).  The number of hot
> keys is limited (I don't think any model has more than 5), so reducing the
> buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> having the size be a power of two).  There seems little point doing this in
> the name of memory saving since the amount saved is trivially small.
> 

It's not a problem as far as I'm concerned. Plenty more lower hanging fruit if
people want to reduce memory footprint.

> Looking through my correspondance with Peter from 2008 (which was largely
> off-list for reasons I do not recall), it seems the reason the kfifo was
> used was due to Peter's concern about concurrent processing of mulitple
> events across different CPUs.  Since at the time we couldn't determine
> whether this was a real issue Peter took the safe route and used a kfifo. 
> If the assumption that it cannot happen is known to hold in all cases there
> is clearly no need for the more complex construct (at least on the basis of
> concurrency arguments).
> 
> I've given some more thought to the following point (from the original patch
> submission):
> > In order to simplify implementation, hotkey input device behavior is
> > slightly changed (from FIFO to LIFO).  The order of release events
> > generated by the input device should not matter from end user
> > perspective as upon releasing any hotkey the firmware only produces a
> > single event which means "all hotkeys were released".
> 
> This will effectively alter the order the button events are seen by
> userspace though, won't it?  It would mean that (for example) a user who
> presses (and holds) the "media" key before "email" will end up with "email"
> being acted on first.  This may surprise them.  Then again, I suppose it
> could be argued that such usage is unusual and therefore is likely to be
> rare - and therefore not worth bothering about.

Michal noted there is only one event to release all keys so the order wouldn't
be noticed in userspace. Has this been confirmed with testing?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Thu, Jun 22, 2017 at 09:20:21AM +0930, Jonathan Woithe wrote:
> On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> > On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > > -#define RINGBUFFERSIZE 40
> > >  
> > >  /* Debugging */
> > >  #define FUJLAPTOP_DBG_ERROR0x0001
> > > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > >   struct input_dev *input;
> > >   char phys[32];
> > >   struct platform_device *pf_device;
> > > - struct kfifo fifo;
> > > - spinlock_t fifo_lock;
> > > + int scancode_buf[40];
> > 
> > Do we know why 40 was used here? A single use magic number is fine, but it
> > would be good to document why it is what it is if we know.
> 
> I am unsure as to why 40 was chosen as the size.  The support for the
> hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
> which featured this functionality (mine doesn't).  I obviously can't speak
> for Peter but I suspect that he simply picked a number which seemed
> reasonable at the time.  I am unaware of any specific reason why the size
> was set at 40, and to be honest never throught to ask at the time the patch
> was proposed.
> 
> If we need a reason to justify the value of 40, I guess the best option is
> "because it seems reasonable".

That's not surprising, and it's fine. If we know, it's worth documenting. If
not, well, we do what we can :-)

> 
> I think the buffer size could probably be reduced a little without impacting
> on functionality.  I suspect the value was chosen so as to be well above the
> number of events which could be generated ahead of a button release (which
> effectively releases all buttons held at that stage).  The number of hot
> keys is limited (I don't think any model has more than 5), so reducing the
> buffer somewhat appears safe (perhaps to 32 if there were any advantages to
> having the size be a power of two).  There seems little point doing this in
> the name of memory saving since the amount saved is trivially small.
> 

It's not a problem as far as I'm concerned. Plenty more lower hanging fruit if
people want to reduce memory footprint.

> Looking through my correspondance with Peter from 2008 (which was largely
> off-list for reasons I do not recall), it seems the reason the kfifo was
> used was due to Peter's concern about concurrent processing of mulitple
> events across different CPUs.  Since at the time we couldn't determine
> whether this was a real issue Peter took the safe route and used a kfifo. 
> If the assumption that it cannot happen is known to hold in all cases there
> is clearly no need for the more complex construct (at least on the basis of
> concurrency arguments).
> 
> I've given some more thought to the following point (from the original patch
> submission):
> > In order to simplify implementation, hotkey input device behavior is
> > slightly changed (from FIFO to LIFO).  The order of release events
> > generated by the input device should not matter from end user
> > perspective as upon releasing any hotkey the firmware only produces a
> > single event which means "all hotkeys were released".
> 
> This will effectively alter the order the button events are seen by
> userspace though, won't it?  It would mean that (for example) a user who
> presses (and holds) the "media" key before "email" will end up with "email"
> being acted on first.  This may surprise them.  Then again, I suppose it
> could be argued that such usage is unusual and therefore is likely to be
> rare - and therefore not worth bothering about.

Michal noted there is only one event to release all keys so the order wouldn't
be noticed in userspace. Has this been confirmed with testing?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > -#define RINGBUFFERSIZE 40
> >  
> >  /* Debugging */
> >  #define FUJLAPTOP_DBG_ERROR  0x0001
> > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > struct input_dev *input;
> > char phys[32];
> > struct platform_device *pf_device;
> > -   struct kfifo fifo;
> > -   spinlock_t fifo_lock;
> > +   int scancode_buf[40];
> 
> Do we know why 40 was used here? A single use magic number is fine, but it
> would be good to document why it is what it is if we know.

I am unsure as to why 40 was chosen as the size.  The support for the
hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
which featured this functionality (mine doesn't).  I obviously can't speak
for Peter but I suspect that he simply picked a number which seemed
reasonable at the time.  I am unaware of any specific reason why the size
was set at 40, and to be honest never throught to ask at the time the patch
was proposed.

If we need a reason to justify the value of 40, I guess the best option is
"because it seems reasonable".

I think the buffer size could probably be reduced a little without impacting
on functionality.  I suspect the value was chosen so as to be well above the
number of events which could be generated ahead of a button release (which
effectively releases all buttons held at that stage).  The number of hot
keys is limited (I don't think any model has more than 5), so reducing the
buffer somewhat appears safe (perhaps to 32 if there were any advantages to
having the size be a power of two).  There seems little point doing this in
the name of memory saving since the amount saved is trivially small.

Looking through my correspondance with Peter from 2008 (which was largely
off-list for reasons I do not recall), it seems the reason the kfifo was
used was due to Peter's concern about concurrent processing of mulitple
events across different CPUs.  Since at the time we couldn't determine
whether this was a real issue Peter took the safe route and used a kfifo. 
If the assumption that it cannot happen is known to hold in all cases there
is clearly no need for the more complex construct (at least on the basis of
concurrency arguments).

I've given some more thought to the following point (from the original patch
submission):
> In order to simplify implementation, hotkey input device behavior is
> slightly changed (from FIFO to LIFO).  The order of release events
> generated by the input device should not matter from end user
> perspective as upon releasing any hotkey the firmware only produces a
> single event which means "all hotkeys were released".

This will effectively alter the order the button events are seen by
userspace though, won't it?  It would mean that (for example) a user who
presses (and holds) the "media" key before "email" will end up with "email"
being acted on first.  This may surprise them.  Then again, I suppose it
could be argued that such usage is unusual and therefore is likely to be
rare - and therefore not worth bothering about.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Jonathan Woithe]

On Wed, Jun 21, 2017 at 11:15:43AM -0700, Darren Hart wrote:
> On Fri, Jun 16, 2017 at 06:40:52AM +0200, Micha?? K??pie?? wrote:
> > -#define RINGBUFFERSIZE 40
> >  
> >  /* Debugging */
> >  #define FUJLAPTOP_DBG_ERROR  0x0001
> > @@ -146,8 +144,8 @@ struct fujitsu_laptop {
> > struct input_dev *input;
> > char phys[32];
> > struct platform_device *pf_device;
> > -   struct kfifo fifo;
> > -   spinlock_t fifo_lock;
> > +   int scancode_buf[40];
> 
> Do we know why 40 was used here? A single use magic number is fine, but it
> would be good to document why it is what it is if we know.

I am unsure as to why 40 was chosen as the size.  The support for the
hotkeys was contributed by Peter Gruber in 2008 who had a Fujitsu laptop
which featured this functionality (mine doesn't).  I obviously can't speak
for Peter but I suspect that he simply picked a number which seemed
reasonable at the time.  I am unaware of any specific reason why the size
was set at 40, and to be honest never throught to ask at the time the patch
was proposed.

If we need a reason to justify the value of 40, I guess the best option is
"because it seems reasonable".

I think the buffer size could probably be reduced a little without impacting
on functionality.  I suspect the value was chosen so as to be well above the
number of events which could be generated ahead of a button release (which
effectively releases all buttons held at that stage).  The number of hot
keys is limited (I don't think any model has more than 5), so reducing the
buffer somewhat appears safe (perhaps to 32 if there were any advantages to
having the size be a power of two).  There seems little point doing this in
the name of memory saving since the amount saved is trivially small.

Looking through my correspondance with Peter from 2008 (which was largely
off-list for reasons I do not recall), it seems the reason the kfifo was
used was due to Peter's concern about concurrent processing of mulitple
events across different CPUs.  Since at the time we couldn't determine
whether this was a real issue Peter took the safe route and used a kfifo. 
If the assumption that it cannot happen is known to hold in all cases there
is clearly no need for the more complex construct (at least on the basis of
concurrency arguments).

I've given some more thought to the following point (from the original patch
submission):
> In order to simplify implementation, hotkey input device behavior is
> slightly changed (from FIFO to LIFO).  The order of release events
> generated by the input device should not matter from end user
> perspective as upon releasing any hotkey the firmware only produces a
> single event which means "all hotkeys were released".

This will effectively alter the order the button events are seen by
userspace though, won't it?  It would mean that (for example) a user who
presses (and holds) the "media" key before "email" will end up with "email"
being acted on first.  This may surprise them.  Then again, I suppose it
could be argued that such usage is unusual and therefore is likely to be
rare - and therefore not worth bothering about.

Regards
  jonathan

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> All ACPI device notify callbacks are invoked using acpi_os_execute(),
> which causes the supplied callback to be queued to a static workqueue
> which always executes on CPU 0.  This means that there is no possibility
> for any ACPI device notify callback to be concurrently executed on
> multiple CPUs, which in the case of fujitsu-laptop means that using a
> locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> of concurrent pushing and popping exists.

Was the kfifo causing a problem currently or for the migration to separate
modules? Is this purely a simplification?

Rafael, the above rationale appears sound to me. Do you have any concerns?

...

> -#define RINGBUFFERSIZE 40
>  
>  /* Debugging */
>  #define FUJLAPTOP_DBG_ERROR0x0001
> @@ -146,8 +144,8 @@ struct fujitsu_laptop {
>   struct input_dev *input;
>   char phys[32];
>   struct platform_device *pf_device;
> - struct kfifo fifo;
> - spinlock_t fifo_lock;
> + int scancode_buf[40];

Do we know why 40 was used here? A single use magic number is fine, but it would
be good to document why it is what it is if we know.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Darren Hart]

On Fri, Jun 16, 2017 at 06:40:52AM +0200, Michał Kępień wrote:
> All ACPI device notify callbacks are invoked using acpi_os_execute(),
> which causes the supplied callback to be queued to a static workqueue
> which always executes on CPU 0.  This means that there is no possibility
> for any ACPI device notify callback to be concurrently executed on
> multiple CPUs, which in the case of fujitsu-laptop means that using a
> locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
> only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
> of concurrent pushing and popping exists.

Was the kfifo causing a problem currently or for the migration to separate
modules? Is this purely a simplification?

Rafael, the above rationale appears sound to me. Do you have any concerns?

...

> -#define RINGBUFFERSIZE 40
>  
>  /* Debugging */
>  #define FUJLAPTOP_DBG_ERROR0x0001
> @@ -146,8 +144,8 @@ struct fujitsu_laptop {
>   struct input_dev *input;
>   char phys[32];
>   struct platform_device *pf_device;
> - struct kfifo fifo;
> - spinlock_t fifo_lock;
> + int scancode_buf[40];

Do we know why 40 was used here? A single use magic number is fine, but it would
be good to document why it is what it is if we know.

-- 
Darren Hart
VMware Open Source Technology Center

[PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

All ACPI device notify callbacks are invoked using acpi_os_execute(),
which causes the supplied callback to be queued to a static workqueue
which always executes on CPU 0.  This means that there is no possibility
for any ACPI device notify callback to be concurrently executed on
multiple CPUs, which in the case of fujitsu-laptop means that using a
locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
of concurrent pushing and popping exists.

Instead of a kfifo, use a fixed-size integer table for storing pushed
scancodes and an associated variable holding the number of scancodes
currently stored in that table.  Change debug messages so that they no
longer contain the term "ringbuffer".

In order to simplify implementation, hotkey input device behavior is
slightly changed (from FIFO to LIFO).  The order of release events
generated by the input device should not matter from end user
perspective as upon releasing any hotkey the firmware only produces a
single event which means "all hotkeys were released".

Signed-off-by: Michał Kępień 
---
 drivers/platform/x86/fujitsu-laptop.c | 54 +--
 1 file changed, 14 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c 
b/drivers/platform/x86/fujitsu-laptop.c
index 1c6fdd952c75..54cb7ae541d4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -58,7 +58,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -110,7 +109,6 @@
 #define KEY5_CODE  0x420
 
 #define MAX_HOTKEY_RINGBUFFER_SIZE 100
-#define RINGBUFFERSIZE 40
 
 /* Debugging */
 #define FUJLAPTOP_DBG_ERROR  0x0001
@@ -146,8 +144,8 @@ struct fujitsu_laptop {
struct input_dev *input;
char phys[32];
struct platform_device *pf_device;
-   struct kfifo fifo;
-   spinlock_t fifo_lock;
+   int scancode_buf[40];
+   int scancode_count;
int flags_supported;
int flags_state;
 };
@@ -813,23 +811,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device 
*device)
sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
device->driver_data = priv;
 
-   /* kfifo */
-   spin_lock_init(>fifo_lock);
-   error = kfifo_alloc(>fifo, RINGBUFFERSIZE * sizeof(int),
-   GFP_KERNEL);
-   if (error) {
-   pr_err("kfifo_alloc failed\n");
-   goto err_stop;
-   }
-
error = acpi_fujitsu_laptop_input_setup(device);
if (error)
-   goto err_free_fifo;
+   return error;
 
error = acpi_bus_update_power(device->handle, );
if (error) {
pr_err("Error reading power state\n");
-   goto err_free_fifo;
+   return error;
}
 
pr_info("ACPI: %s [%s] (%s)\n",
@@ -877,62 +866,47 @@ static int acpi_fujitsu_laptop_add(struct acpi_device 
*device)
 
error = acpi_fujitsu_laptop_leds_register(device);
if (error)
-   goto err_free_fifo;
+   return error;
 
error = fujitsu_laptop_platform_add(device);
if (error)
-   goto err_free_fifo;
+   return error;
 
return 0;
-
-err_free_fifo:
-   kfifo_free(>fifo);
-err_stop:
-   return error;
 }
 
 static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
-   struct fujitsu_laptop *priv = acpi_driver_data(device);
-
fujitsu_laptop_platform_remove(device);
 
-   kfifo_free(>fifo);
-
return 0;
 }
 
 static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
 {
struct fujitsu_laptop *priv = acpi_driver_data(device);
-   int status;
 
-   status = kfifo_in_locked(>fifo, (unsigned char *),
-sizeof(scancode), >fifo_lock);
-   if (status != sizeof(scancode)) {
+   if (priv->scancode_count == ARRAY_SIZE(priv->scancode_buf)) {
vdbg_printk(FUJLAPTOP_DBG_WARN,
"Could not push scancode [0x%x]\n", scancode);
return;
}
+   priv->scancode_buf[priv->scancode_count++] = scancode;
sparse_keymap_report_event(priv->input, scancode, 1, false);
vdbg_printk(FUJLAPTOP_DBG_TRACE,
-   "Push scancode into ringbuffer [0x%x]\n", scancode);
+   "Push scancode into buffer [0x%x]\n", scancode);
 }
 
 static void acpi_fujitsu_laptop_release(struct acpi_device *device)
 {
struct fujitsu_laptop *priv = acpi_driver_data(device);
-   int scancode, status;
-
-   while (true) {
-   status = kfifo_out_locked(>fifo,
- (unsigned char *),
- sizeof(scancode), >fifo_lock);
-   if (status != sizeof(scancode))
- 

[PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes

[Michał Kępień]

All ACPI device notify callbacks are invoked using acpi_os_execute(),
which causes the supplied callback to be queued to a static workqueue
which always executes on CPU 0.  This means that there is no possibility
for any ACPI device notify callback to be concurrently executed on
multiple CPUs, which in the case of fujitsu-laptop means that using a
locked kfifo for handling hotkeys is redundant: as hotkey scancodes are
only pushed and popped from within acpi_fujitsu_laptop_notify(), no risk
of concurrent pushing and popping exists.

Instead of a kfifo, use a fixed-size integer table for storing pushed
scancodes and an associated variable holding the number of scancodes
currently stored in that table.  Change debug messages so that they no
longer contain the term "ringbuffer".

In order to simplify implementation, hotkey input device behavior is
slightly changed (from FIFO to LIFO).  The order of release events
generated by the input device should not matter from end user
perspective as upon releasing any hotkey the firmware only produces a
single event which means "all hotkeys were released".

Signed-off-by: Michał Kępień 
---
 drivers/platform/x86/fujitsu-laptop.c | 54 +--
 1 file changed, 14 insertions(+), 40 deletions(-)

diff --git a/drivers/platform/x86/fujitsu-laptop.c 
b/drivers/platform/x86/fujitsu-laptop.c
index 1c6fdd952c75..54cb7ae541d4 100644
--- a/drivers/platform/x86/fujitsu-laptop.c
+++ b/drivers/platform/x86/fujitsu-laptop.c
@@ -58,7 +58,6 @@
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -110,7 +109,6 @@
 #define KEY5_CODE  0x420
 
 #define MAX_HOTKEY_RINGBUFFER_SIZE 100
-#define RINGBUFFERSIZE 40
 
 /* Debugging */
 #define FUJLAPTOP_DBG_ERROR  0x0001
@@ -146,8 +144,8 @@ struct fujitsu_laptop {
struct input_dev *input;
char phys[32];
struct platform_device *pf_device;
-   struct kfifo fifo;
-   spinlock_t fifo_lock;
+   int scancode_buf[40];
+   int scancode_count;
int flags_supported;
int flags_state;
 };
@@ -813,23 +811,14 @@ static int acpi_fujitsu_laptop_add(struct acpi_device 
*device)
sprintf(acpi_device_class(device), "%s", ACPI_FUJITSU_CLASS);
device->driver_data = priv;
 
-   /* kfifo */
-   spin_lock_init(>fifo_lock);
-   error = kfifo_alloc(>fifo, RINGBUFFERSIZE * sizeof(int),
-   GFP_KERNEL);
-   if (error) {
-   pr_err("kfifo_alloc failed\n");
-   goto err_stop;
-   }
-
error = acpi_fujitsu_laptop_input_setup(device);
if (error)
-   goto err_free_fifo;
+   return error;
 
error = acpi_bus_update_power(device->handle, );
if (error) {
pr_err("Error reading power state\n");
-   goto err_free_fifo;
+   return error;
}
 
pr_info("ACPI: %s [%s] (%s)\n",
@@ -877,62 +866,47 @@ static int acpi_fujitsu_laptop_add(struct acpi_device 
*device)
 
error = acpi_fujitsu_laptop_leds_register(device);
if (error)
-   goto err_free_fifo;
+   return error;
 
error = fujitsu_laptop_platform_add(device);
if (error)
-   goto err_free_fifo;
+   return error;
 
return 0;
-
-err_free_fifo:
-   kfifo_free(>fifo);
-err_stop:
-   return error;
 }
 
 static int acpi_fujitsu_laptop_remove(struct acpi_device *device)
 {
-   struct fujitsu_laptop *priv = acpi_driver_data(device);
-
fujitsu_laptop_platform_remove(device);
 
-   kfifo_free(>fifo);
-
return 0;
 }
 
 static void acpi_fujitsu_laptop_press(struct acpi_device *device, int scancode)
 {
struct fujitsu_laptop *priv = acpi_driver_data(device);
-   int status;
 
-   status = kfifo_in_locked(>fifo, (unsigned char *),
-sizeof(scancode), >fifo_lock);
-   if (status != sizeof(scancode)) {
+   if (priv->scancode_count == ARRAY_SIZE(priv->scancode_buf)) {
vdbg_printk(FUJLAPTOP_DBG_WARN,
"Could not push scancode [0x%x]\n", scancode);
return;
}
+   priv->scancode_buf[priv->scancode_count++] = scancode;
sparse_keymap_report_event(priv->input, scancode, 1, false);
vdbg_printk(FUJLAPTOP_DBG_TRACE,
-   "Push scancode into ringbuffer [0x%x]\n", scancode);
+   "Push scancode into buffer [0x%x]\n", scancode);
 }
 
 static void acpi_fujitsu_laptop_release(struct acpi_device *device)
 {
struct fujitsu_laptop *priv = acpi_driver_data(device);
-   int scancode, status;
-
-   while (true) {
-   status = kfifo_out_locked(>fifo,
- (unsigned char *),
- sizeof(scancode), >fifo_lock);
-   if (status != sizeof(scancode))
-   return;