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

Groups > linux.kernel > #1671898 > unrolled thread

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

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Darren Hart <dvhart@infradead.org> - 2017-06-21 20:20 +0200
    Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Jonathan Woithe <jwoithe@just42.net> - 2017-06-22 02:00 +0200
      Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Darren Hart <dvhart@infradead.org> - 2017-06-22 04:50 +0200
        Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Jonathan Woithe <jwoithe@just42.net> - 2017-06-22 05:10 +0200
          Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Michał Kępień <kernel@kempniu.pl> - 2017-06-22 22:50 +0200
            Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Darren Hart <dvhart@infradead.org> - 2017-06-23 02:00 +0200
              Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Jonathan Woithe <jwoithe@just42.net> - 2017-06-23 02:20 +0200
                Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Darren Hart <dvhart@infradead.org> - 2017-06-23 08:00 +0200
    Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Michał Kępień <kernel@kempniu.pl> - 2017-06-22 22:10 +0200
    Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for storing hotkey scancodes "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-06-24 02:40 +0200
      Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Darren Hart <dvhart@infradead.org> - 2017-06-27 02:10 +0200
        Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Jonathan Woithe <jwoithe@just42.net> - 2017-06-27 14:20 +0200
        Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Michał Kępień <kernel@kempniu.pl> - 2017-06-28 06:40 +0200
          Re: [PATCH 1/7] platform/x86: fujitsu-laptop: do not use kfifo for  storing hotkey scancodes Darren Hart <dvhart@infradead.org> - 2017-06-28 18:10 +0200

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

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

-- 
Darren Hart
VMware Open Source Technology Center

[toc] | [next] | [standalone]

#1672166

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

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

#1672266

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_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".

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

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

#1672270

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

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

#1673037

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

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

#1673138

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

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

#1673149

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

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

#1673278

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

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

#1673021

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

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

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

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

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

#1675203

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

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

#1675615

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

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

#1676363

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

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

#1676903

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web