| Message ID | 20170616044058.30443-2-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
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.
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
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?
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
> 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.
> 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.
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.
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
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.
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
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,
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
> 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.
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.
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 <linux/fb.h> #include <linux/input.h> #include <linux/input/sparse-keymap.h> -#include <linux/kfifo.h> #include <linux/leds.h> #include <linux/platform_device.h> #include <linux/slab.h> @@ -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(&priv->fifo_lock); - error = kfifo_alloc(&priv->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, &state); 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(&priv->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(&priv->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(&priv->fifo, (unsigned char *)&scancode, - sizeof(scancode), &priv->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(&priv->fifo, - (unsigned char *)&scancode, - sizeof(scancode), &priv->fifo_lock); - if (status != sizeof(scancode)) - return; + int scancode; + + while (priv->scancode_count > 0) { + scancode = priv->scancode_buf[--priv->scancode_count]; sparse_keymap_report_event(priv->input, scancode, 0, false); vdbg_printk(FUJLAPTOP_DBG_TRACE, - "Pop scancode from ringbuffer [0x%x]\n", scancode); + "Pop scancode from buffer [0x%x]\n", scancode); } }
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ń <kernel@kempniu.pl> --- drivers/platform/x86/fujitsu-laptop.c | 54 +++++++++-------------------------- 1 file changed, 14 insertions(+), 40 deletions(-)