lkml.org 
[lkml]   [2018]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/7] platform/x86: fujitsu-laptop: More accurately represent the hotkey ring buffer managed by firmware
> On Tue, Feb 27, 2018 at 11:15 PM, Michał Kępień <kernel@kempniu.pl> wrote:
> > The MAX_HOTKEY_RINGBUFFER_SIZE constant is set to 100, which allows up
> > to 100 hotkey events to be drained from the firmware ring buffer upon
> > module load. However, no known firmware is capable of holding more than
> > 16 hotkey events in its internal ring buffer:
>
> > The RINGBUFFERSIZE constant is set to 40, which allows the module to
> > queue up to 40 hotkey events for delivery to user space. As this value
> > seems arbitrarily chosen and 16 should be more than enough for any
> > practical use case, merge the two aforementioned constants into one
> > (HOTKEY_RINGBUFFER_SIZE) in order to simplify code and more accurately
> > represent the underlying data structure.
>
> > +#define HOTKEY_RINGBUFFER_SIZE 16
>
> This need the comment or a
>
> BUILD_BUG_ON(!is_power_of_2(...));

Okay, I will probably take the comment route in v2.

> > - ret = kfifo_alloc(&priv->fifo, RINGBUFFERSIZE * sizeof(int),
> > + ret = kfifo_alloc(&priv->fifo, HOTKEY_RINGBUFFER_SIZE * sizeof(int),
> > GFP_KERNEL);
>
> > while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
> > 0x0, 0x0) != 0 &&
> > - i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
> > + i++ < HOTKEY_RINGBUFFER_SIZE)
> > ; /* No action, result is discarded */
>
> This looks horrible.

It sure does! Hence patch 7/7, which does the following:

- while (call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
- 0x0, 0x0) != 0 &&
+ while (fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0) != 0 &&
i++ < HOTKEY_RINGBUFFER_SIZE)

In other words, patch 6/7 is just a stopover on the way to shorten
current module code:

- while (call_fext_func(device, FUNC_BUTTONS, 0x1, 0x0, 0x0) != 0 &&
- i++ < MAX_HOTKEY_RINGBUFFER_SIZE)
+ while (fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0) != 0 &&
+ i++ < HOTKEY_RINGBUFFER_SIZE)

> > while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
> > 0x0, 0x0)) != 0 &&
> > - i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
> > + i++ < HOTKEY_RINGBUFFER_SIZE) {
>
> Ditto.

Similarly, patch 7/7 does the following:

- while ((irb = call_fext_func(device, FUNC_BUTTONS, OP_GET_EVENTS,
- 0x0, 0x0)) != 0 &&
+ while ((irb = fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0)) != 0 &&
i++ < HOTKEY_RINGBUFFER_SIZE) {

The diff against current module code is thus:

- while ((irb = call_fext_func(device,
- FUNC_BUTTONS, 0x1, 0x0, 0x0)) != 0 &&
- i++ < MAX_HOTKEY_RINGBUFFER_SIZE) {
+ while ((irb = fext_buttons(device, OP_GET_EVENTS, 0x0, 0x0)) != 0 &&
+ i++ < HOTKEY_RINGBUFFER_SIZE) {

--
Best regards,
Michał Kępień

\
 
 \ /
  Last update: 2018-03-04 20:57    [W:0.057 / U:10.340 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site