Re: [PATCH] dell-wmi: Update code for processing WMI events

From: Pali RohÃr
Date: Sun Nov 09 2014 - 10:22:30 EST

Next message: Philipp Zabel: "Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint"
Previous message: Jiang Liu: "[RFC Part4 v1 17/17] x86, htirq: Use common MSI code to manage Hypertransport interrupts"
Next in thread: Darren Hart: "Re: [PATCH] dell-wmi: Update code for processing WMI events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wednesday 22 October 2014 12:51:17 Pali RohÃr wrote:
> On Tuesday 21 October 2014 23:32:12 Darren Hart wrote:
> > On Tue, Oct 21, 2014 at 12:15:24AM +0200, Pali RohÃr wrote:
> > > WMI buffer can contains more events. First value in buffer
> > > is length of event followed by data of specified length.
> > > After that is next length and next data. When length is
> > > zero then there is no more events in bufffer.
> > >
> > > This patch adds support for processing all events in
> > > buffer (not only first) and parse more event types (not
> > > only hotkey events). Because of variable length of events
> > > sometimes BIOS fills more hotkeys (or other values) into
> > > single WMI event. In this case this patch process also
> > > these multiple hotkeys (and not only first one).
> > >
> > > Some event types are just ignored because kernel is not
> > > interested for them (e.g. NIC Link status, battery unplug,
> > > ...).
> > >
> > > This patch is based on DSDT table from Dell Latitude
> > > E6440. Code should be backward compatible so will process
> > > other events of old types same as before this patch.
> > >
> > > This patch also fixes problem when in kernel log are
> > > written messages about unknown WMI events. Now all know
> > > events are parsed and those which are not interesting for
> > > kernel are dropped without unknown message.
> >
> > This should probably be done in a separate patch.
>
> It is not possible, because my patch rewrite code for handling
> events. Kernel does not print "unknown event" messages when it
> parse WMI event and understand specified part.
>
> > > Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
> > > Tested-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
> >
> > Well yes, I should hope so ;-)
> >
> > > ---
> > >
> > > drivers/platform/x86/dell-wmi.c | 157
> > > +++++++++++++++++++++++++++++++-------- 1 file changed,
> > > 127 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/dell-wmi.c
> > > b/drivers/platform/x86/dell-wmi.c index 25721bf..3d15949
> > > 100644
> > > --- a/drivers/platform/x86/dell-wmi.c
> > > +++ b/drivers/platform/x86/dell-wmi.c
> > > @@ -145,11 +145,35 @@ static const u16
> > > bios_to_linux_keycode[256] __initconst = {
> > >
> > > static struct input_dev *dell_wmi_input_dev;
> > >
> > > +static void dell_wmi_process_key(int reported_key)
> > > +{
> > > + const struct key_entry *key;
> > > +
> > > + key =
> > > sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
> > > + reported_key);
> > > + if (!key) {
> > > + pr_info("Unknown key %x pressed\n", reported_key);
> > > + return;
> > > + }
> > > +
> > > + pr_debug("Key %x pressed\n", reported_key);
> > > +
> > > + /* Don't report brightness notifications that will also
> > > come via ACPI */ + if ((key->keycode == KEY_BRIGHTNESSUP
> > > || + key->keycode == KEY_BRIGHTNESSDOWN) &&
> > > acpi_video) + return;
> > > +
> > > + sparse_keymap_report_entry(dell_wmi_input_dev, key, 1,
> > > true); +}
> > > +
> > >
> > > static void dell_wmi_notify(u32 value, void *context)
> > > {
> > >
> > > struct acpi_buffer response = { ACPI_ALLOCATE_BUFFER,
> > > NULL }; union acpi_object *obj;
> > > acpi_status status;
> > >
> > > + acpi_size buffer_size;
> > > + u16 *buffer_entry, *buffer_end;
> > > + int len, i;
> > >
> > > status = wmi_get_event_data(value, &response);
> > > if (status != AE_OK) {
> > >
> > > @@ -158,44 +182,117 @@ static void dell_wmi_notify(u32
> > > value, void *context)
> > >
> > > }
> > >
> > > obj = (union acpi_object *)response.pointer;
> > >
> > > + if (!obj) {
> > > + pr_info("no response\n");
> > > + return;
> > > + }
> >
> > If you intend to print this, it should probably be a bit
> > more informative. Is "info" the right level here? I would
> > imagine either WARN if this was a bad thing, or DEBUG if
> > this is more for debugging the driver.
>
> So what you (or somebody else) prefer? warn or debug?
>
> > > - if (obj && obj->type == ACPI_TYPE_BUFFER) {
> > > - const struct key_entry *key;
> > > - int reported_key;
> > > - u16 *buffer_entry = (u16 *)obj->buffer.pointer;
> > > - int buffer_size = obj->buffer.length/2;
> > > -
> > > - if (buffer_size >= 2 && dell_new_hk_type &&
> > > buffer_entry[1] != 0x10) { - pr_info("Received
>
> unknown
>
> > > WMI event (0x%x)\n",
> > > - buffer_entry[1]);
> > > - kfree(obj);
> > > - return;
> > > - }
> > > + if (obj->type != ACPI_TYPE_BUFFER) {
> > > + pr_info("bad response type %x\n", obj->type);
> > > + kfree(obj);
> > > + return;
> > > + }
> > > +
> > > + pr_debug("Received WMI event (%*ph)\n",
> > > + obj->buffer.length, obj->buffer.pointer);
> > >
> > > - if (buffer_size >= 3 && (dell_new_hk_type ||
> > > buffer_entry[1] == 0x0)) - reported_key =
> > > (int)buffer_entry[2];
> > > + buffer_entry = (u16 *)obj->buffer.pointer;
> > > + buffer_size = obj->buffer.length/2;
> > > +
> > > + if (!dell_new_hk_type) {
> > > + if (buffer_size >= 3 && buffer_entry[1] == 0x0)
> > > + dell_wmi_process_key(buffer_entry[2]);
> > >
> > > else if (buffer_size >= 2)
> > >
> > > - reported_key = (int)buffer_entry[1] & 0xffff;
> > > - else {
> > > + dell_wmi_process_key(buffer_entry[1]);
> >
> > Why can we drop the 0xffff mask now?
>
> Because it is useless (or correct me if not!). Variable
> buffer_entry has type u16* so operation "AND 0xFFFF" on 16bit
> integer do nothing.
>
> > > + else
> > >
> > > pr_info("Received unknown WMI event\n");
> > >
> > > - kfree(obj);
> > > - return;
> > > + kfree(obj);
> > > + return;
> > > + }
> > > +
> > > + buffer_end = buffer_entry + buffer_size;
> > > +
> > > + while (buffer_entry < buffer_end) {
> > > +
> > > + len = buffer_entry[0];
> > > + if (len == 0)
> > > + break;
> > > +
> > > + len++;
> > > +
> >
> > Why increment len here? Are you trying to avoid a "len + 1"
> > in the comparisons below? If so, is using "len * 2" in the
> > debug message below correct? Please clarify.
>
> in buffer_entry[0] (16 bit integer) is stored length of event
> (in 16bit units) without first (length) value. And "%*ph"
> takes size in bytes (u8). So length in bytes (u8) units is 2
> * length in u16 units.
>
> > > + if (buffer_entry+len > buffer_end) {
> >
> > See coding style documentation on operators. Please run
> > patches through checkpatch.
>
> checkpatch.pl does not show any problem for these lines.
>
> > > + pr_info("Invalid length of WMI event\n");
> >
> > info doesn't see correct here either.
>
> debug or warn?
>
> > > + break;
> > >
> > > }
> > >
> > > - key =
> > > sparse_keymap_entry_from_scancode(dell_wmi_input_dev,
> > > - reported_key);
> > > - if (!key) {
> > > - pr_info("Unknown key %x pressed\n",
reported_key);
> > > - } else if ((key->keycode == KEY_BRIGHTNESSUP ||
> > > - key->keycode == KEY_BRIGHTNESSDOWN) &&
>
> acpi_video) {
>
> > > - /* Don't report brightness notifications that
will
>
> also
>
> > > - * come via ACPI */
> > > - ;
> > > - } else {
> > > - sparse_keymap_report_entry(dell_wmi_input_dev,
>
> key,
>
> > > - 1, true);
> > > + pr_debug("Process buffer (%*ph)\n", len*2,
>
> buffer_entry);
>
> > > +
> > > + switch (buffer_entry[1]) {
> > > + case 0x00:
> > > + for (i = 2; i < len; ++i) {
> > > + switch (buffer_entry[i]) {
> > > + case 0xe043:
> > > + /* NIC Link is Up */
> > > + pr_debug("NIC Link is Up\n");
> > > + break;
> > > + case 0xe044:
> > > + /* NIC Link is Down */
> > > + pr_debug("NIC Link is Down\n");
> > > + break;
> > > + case 0xe045:
> > > + /* Unknown event but defined in DSDT */
> > > + default:
> > > + /* Unknown event */
> > > + pr_info("Unknown WMI event type 0x00: "
> > > + "0x%x\n", (int)buffer_entry[i]);
> > > + break;
> > > + }
> > > + }
> > > + break;
> > > + case 0x10:
> > > + /* Keys pressed */
> > > + for (i = 2; i < len; ++i)
> > > + dell_wmi_process_key(buffer_entry[i]);
> > > + break;
> > > + case 0x11:
> > > + for (i = 2; i < len; ++i) {
> > > + switch (buffer_entry[i]) {
> > > + case 0xfff0:
> > > + /* Battery unplugged */
> > > + pr_debug("Battery unplugged\n");
> > > + break;
> > > + case 0xfff1:
> > > + /* Battery inserted */
> > > + pr_debug("Battery inserted\n");
> > > + break;
> > > + case 0x01e1:
> > > + case 0x02ea:
> > > + case 0x02eb:
> > > + case 0x02ec:
> > > + case 0x02f6:
> > > + /* Keyboard backlight level changed */
> > > + pr_debug("Keyboard backlight level "
> > > + "changed\n");
> > > + break;
> > > + default:
> > > + /* Unknown event */
> > > + pr_info("Unknown WMI event type 0x11: "
> > > + "0x%x\n", (int)buffer_entry[i]);
> > > + break;
> > > + }
> > > + }
> > > + break;
> > > + default:
> > > + /* Unknown event */
> > > + pr_info("Unknown WMI event type 0x%x\n",
> > > + (int)buffer_entry[1]);
> > > + break;
> > >
> > > }
> > >
> > > +
> > > + buffer_entry += len;
> > > +
> > >
> > > }
> > >
> > > +
> > >
> > > kfree(obj);
> > >
> > > }

Darren: PING. See my comments and questions.

--
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: This is a digitally signed message part.

Next message: Philipp Zabel: "Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint"
Previous message: Jiang Liu: "[RFC Part4 v1 17/17] x86, htirq: Use common MSI code to manage Hypertransport interrupts"
Next in thread: Darren Hart: "Re: [PATCH] dell-wmi: Update code for processing WMI events"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]