Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Mon, Feb 22, 2016 at 09:56:50AM +0100, Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >   int err;
> > >   acpi_status status;
> > > + struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last 
> > declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x1 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.
> 
> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

I suggested inline because the code was inline before and there would be no size
overhead to continue to make it inline. That said, the best source of guidance
on this is in CodingStyle, Chapter 15. While this function is quite small and
static to the file, it is not performance critical as you say. So, upon closer
inspection, there is no real need to be inline. And in fact, as there is no need
for it, it perhaps should not be. Thanks for raising the question.

> 
> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

Great, thank you.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Mon, Feb 22, 2016 at 09:56:50AM +0100, Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >   int err;
> > >   acpi_status status;
> > > + struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last 
> > declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x1 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.
> 
> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

I suggested inline because the code was inline before and there would be no size
overhead to continue to make it inline. That said, the best source of guidance
on this is in CodingStyle, Chapter 15. While this function is quite small and
static to the file, it is not performance critical as you say. So, upon closer
inspection, there is no real need to be inline. And in fact, as there is no need
for it, it perhaps should not be. Thanks for raising the question.

> 
> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

Great, thank you.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> > > Pali's point about documenting the hardcoded values and eliminating the 
> > > code
> > > duplication with a function (inline) is a good one.
> > 
> > I plan to only put a comment next to 0x51534554 as 0x1 is apparently
> > just something pulled out of a hat (as the link provided in the commit
> > message proves) and input[3] should be self-explanatory due to the name
> > of the variable whose value is put into it.
> 
> Maybe you can add documentation which we got from Dell on some ML about
> this SMI call. Similarly what I added in dell-laptop.c...

Sure, I can do that.

> > By the way, is there any kernel-wide or subsystem-wide policy for
> > marking a function inline?  I mean, this is hardly time-critical code,
> > so is your suggestion to make it inline just a preference or am I
> > unaware of some rule?
> 
> IIRC recent versions of gcc ignores "inline" keyword and inline
> functions as needed when doing optimizations.

This was my hunch as well, but I couldn't find any proof immediately,
hence the question.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> > > Pali's point about documenting the hardcoded values and eliminating the 
> > > code
> > > duplication with a function (inline) is a good one.
> > 
> > I plan to only put a comment next to 0x51534554 as 0x1 is apparently
> > just something pulled out of a hat (as the link provided in the commit
> > message proves) and input[3] should be self-explanatory due to the name
> > of the variable whose value is put into it.
> 
> Maybe you can add documentation which we got from Dell on some ML about
> this SMI call. Similarly what I added in dell-laptop.c...

Sure, I can do that.

> > By the way, is there any kernel-wide or subsystem-wide policy for
> > marking a function inline?  I mean, this is hardly time-critical code,
> > so is your suggestion to make it inline just a preference or am I
> > unaware of some rule?
> 
> IIRC recent versions of gcc ignores "inline" keyword and inline
> functions as needed when doing optimizations.

This was my hunch as well, but I couldn't find any proof immediately,
hence the question.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Monday 22 February 2016 09:56:50 Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >   int err;
> > >   acpi_status status;
> > > + struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last 
> > declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x1 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.

Maybe you can add documentation which we got from Dell on some ML about
this SMI call. Similarly what I added in dell-laptop.c...

> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

IIRC recent versions of gcc ignores "inline" keyword and inline
functions as needed when doing optimizations.

If there is some functions which must be inlined you need to to use gcc
attrbiute always_inline.

But if there is policy? I do not know, maybe somebody else should
comment it.

> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Monday 22 February 2016 09:56:50 Michał Kępień wrote:
> > >  /*
> > >   * Certain keys are flagged as KE_IGNORE. All of these are either
> > >   * notifications (rather than requests for change) or are also sent
> > > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> > >  {
> > >   int err;
> > >   acpi_status status;
> > > + struct calling_interface_buffer *buffer;
> > 
> > Please place the longest line first, and move int err to the last 
> > declaration.
> > When changing declarations of local variables, please use "Reverse Christmas
> > Tree" order (longest line to shortest line) wherever possible.
> 
> Thanks, I'll keep that in mind for the future, though putting the
> WMI-enabling SMBIOS request in a separate function renders the need for
> the buffer variable in dell_wmi_init() void, so v4 won't touch this area
> any more.
> 
> > Pali's point about documenting the hardcoded values and eliminating the code
> > duplication with a function (inline) is a good one.
> 
> I plan to only put a comment next to 0x51534554 as 0x1 is apparently
> just something pulled out of a hat (as the link provided in the commit
> message proves) and input[3] should be self-explanatory due to the name
> of the variable whose value is put into it.

Maybe you can add documentation which we got from Dell on some ML about
this SMI call. Similarly what I added in dell-laptop.c...

> By the way, is there any kernel-wide or subsystem-wide policy for
> marking a function inline?  I mean, this is hardly time-critical code,
> so is your suggestion to make it inline just a preference or am I
> unaware of some rule?

IIRC recent versions of gcc ignores "inline" keyword and inline
functions as needed when doing optimizations.

If there is some functions which must be inlined you need to to use gcc
attrbiute always_inline.

But if there is policy? I do not know, maybe somebody else should
comment it.

> > Otherwise, this series looks good to me. Looking forward to merging v4.
> 
> I'll try to post a v4 within the next couple of days.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> >  /*
> >   * Certain keys are flagged as KE_IGNORE. All of these are either
> >   * notifications (rather than requests for change) or are also sent
> > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> >  {
> > int err;
> > acpi_status status;
> > +   struct calling_interface_buffer *buffer;
> 
> Please place the longest line first, and move int err to the last declaration.
> When changing declarations of local variables, please use "Reverse Christmas
> Tree" order (longest line to shortest line) wherever possible.

Thanks, I'll keep that in mind for the future, though putting the
WMI-enabling SMBIOS request in a separate function renders the need for
the buffer variable in dell_wmi_init() void, so v4 won't touch this area
any more.

> Pali's point about documenting the hardcoded values and eliminating the code
> duplication with a function (inline) is a good one.

I plan to only put a comment next to 0x51534554 as 0x1 is apparently
just something pulled out of a hat (as the link provided in the commit
message proves) and input[3] should be self-explanatory due to the name
of the variable whose value is put into it.

By the way, is there any kernel-wide or subsystem-wide policy for
marking a function inline?  I mean, this is hardly time-critical code,
so is your suggestion to make it inline just a preference or am I
unaware of some rule?

> Otherwise, this series looks good to me. Looking forward to merging v4.

I'll try to post a v4 within the next couple of days.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> >  /*
> >   * Certain keys are flagged as KE_IGNORE. All of these are either
> >   * notifications (rather than requests for change) or are also sent
> > @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
> >  {
> > int err;
> > acpi_status status;
> > +   struct calling_interface_buffer *buffer;
> 
> Please place the longest line first, and move int err to the last declaration.
> When changing declarations of local variables, please use "Reverse Christmas
> Tree" order (longest line to shortest line) wherever possible.

Thanks, I'll keep that in mind for the future, though putting the
WMI-enabling SMBIOS request in a separate function renders the need for
the buffer variable in dell_wmi_init() void, so v4 won't touch this area
any more.

> Pali's point about documenting the hardcoded values and eliminating the code
> duplication with a function (inline) is a good one.

I plan to only put a comment next to 0x51534554 as 0x1 is apparently
just something pulled out of a hat (as the link provided in the commit
message proves) and input[3] should be self-explanatory due to the name
of the variable whose value is put into it.

By the way, is there any kernel-wide or subsystem-wide policy for
marking a function inline?  I mean, this is hardly time-critical code,
so is your suggestion to make it inline just a preference or am I
unaware of some rule?

> Otherwise, this series looks good to me. Looking forward to merging v4.

I'll try to post a v4 within the next couple of days.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Tue, Feb 16, 2016 at 03:50:28PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), WMI events are not
> generated until a specific SMBIOS request is issued to register an event
> listener [1].  As there seems to be no ACPI method or SMBIOS request to
> determine without possible side effects whether a given machine needs to
> issue this SMBIOS request in order to receive WMI events, DMI matching
> is used to whitelist the models which need it.
> 
> [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html
> 
> Signed-off-by: Michał Kępień 
> ---
>  drivers/platform/x86/Kconfig|1 +
>  drivers/platform/x86/dell-wmi.c |   49 
> +++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3e4d9c3..5ceb53a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -122,6 +122,7 @@ config DELL_WMI
>   depends on ACPI_WMI
>   depends on INPUT
>   depends on ACPI_VIDEO || ACPI_VIDEO = n
> + depends on DELL_SMBIOS
>   select INPUT_SPARSEKMAP
>   ---help---
> Say Y here if you want to support WMI-based hotkeys on Dell laptops.
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 368e193..ca8233a 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include "dell-smbios.h"
>  
>  MODULE_AUTHOR("Matthew Garrett ");
>  MODULE_AUTHOR("Pali Rohár ");
> @@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
>  #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
>  static u32 dell_wmi_interface_version;
> +static bool wmi_requires_smbios_request;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
>  
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> + wmi_requires_smbios_request = 1;
> + return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Vostro V131",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> + },
> + },
> + { }
> +};
> +
>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
>  {
>   int err;
>   acpi_status status;
> + struct calling_interface_buffer *buffer;

Please place the longest line first, and move int err to the last declaration.
When changing declarations of local variables, please use "Reverse Christmas
Tree" order (longest line to shortest line) wherever possible.

>  
>   if (!wmi_has_guid(DELL_EVENT_GUID) ||
>   !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
> @@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
>   return -ENODEV;
>   }
>  
> + dmi_check_system(dell_wmi_smbios_list);
> +
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + buffer->input[3] = 0x1;
> + dell_smbios_send_request(17, 3);
> + err = buffer->output[0];
> + dell_smbios_release_buffer();
> + if (err) {
> + pr_err("Failed to enable WMI (error %d)\n", err);
> + wmi_remove_notify_handler(DELL_EVENT_GUID);
> + dell_wmi_input_destroy();
> + return dell_smbios_error(err);
> + }
> + }
> +
>   return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> + struct calling_interface_buffer *buffer;
> +
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + dell_smbios_send_request(17, 3);
> + dell_smbios_release_buffer();
> + }
> +

Pali's point about documenting the hardcoded values and eliminating the code
duplication with a function (inline) is a good one.

Otherwise, this series looks good to me. Looking forward to merging v4.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Darren Hart]

On Tue, Feb 16, 2016 at 03:50:28PM +0100, Michał Kępień wrote:
> On some laptop models (e.g. Dell Vostro V131), WMI events are not
> generated until a specific SMBIOS request is issued to register an event
> listener [1].  As there seems to be no ACPI method or SMBIOS request to
> determine without possible side effects whether a given machine needs to
> issue this SMBIOS request in order to receive WMI events, DMI matching
> is used to whitelist the models which need it.
> 
> [1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html
> 
> Signed-off-by: Michał Kępień 
> ---
>  drivers/platform/x86/Kconfig|1 +
>  drivers/platform/x86/dell-wmi.c |   49 
> +++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 3e4d9c3..5ceb53a 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -122,6 +122,7 @@ config DELL_WMI
>   depends on ACPI_WMI
>   depends on INPUT
>   depends on ACPI_VIDEO || ACPI_VIDEO = n
> + depends on DELL_SMBIOS
>   select INPUT_SPARSEKMAP
>   ---help---
> Say Y here if you want to support WMI-based hotkeys on Dell laptops.
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index 368e193..ca8233a 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -37,6 +37,7 @@
>  #include 
>  #include 
>  #include 
> +#include "dell-smbios.h"
>  
>  MODULE_AUTHOR("Matthew Garrett ");
>  MODULE_AUTHOR("Pali Rohár ");
> @@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
>  #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
>  
>  static u32 dell_wmi_interface_version;
> +static bool wmi_requires_smbios_request;
>  
>  MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>  MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
>  
> +static int __init dmi_matched(const struct dmi_system_id *dmi)
> +{
> + wmi_requires_smbios_request = 1;
> + return 1;
> +}
> +
> +static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
> + {
> + .callback = dmi_matched,
> + .ident = "Dell Vostro V131",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> + },
> + },
> + { }
> +};
> +
>  /*
>   * Certain keys are flagged as KE_IGNORE. All of these are either
>   * notifications (rather than requests for change) or are also sent
> @@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
>  {
>   int err;
>   acpi_status status;
> + struct calling_interface_buffer *buffer;

Please place the longest line first, and move int err to the last declaration.
When changing declarations of local variables, please use "Reverse Christmas
Tree" order (longest line to shortest line) wherever possible.

>  
>   if (!wmi_has_guid(DELL_EVENT_GUID) ||
>   !wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
> @@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
>   return -ENODEV;
>   }
>  
> + dmi_check_system(dell_wmi_smbios_list);
> +
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + buffer->input[3] = 0x1;
> + dell_smbios_send_request(17, 3);
> + err = buffer->output[0];
> + dell_smbios_release_buffer();
> + if (err) {
> + pr_err("Failed to enable WMI (error %d)\n", err);
> + wmi_remove_notify_handler(DELL_EVENT_GUID);
> + dell_wmi_input_destroy();
> + return dell_smbios_error(err);
> + }
> + }
> +
>   return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> + struct calling_interface_buffer *buffer;
> +
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + dell_smbios_send_request(17, 3);
> + dell_smbios_release_buffer();
> + }
> +

Pali's point about documenting the hardcoded values and eliminating the code
duplication with a function (inline) is a good one.

Otherwise, this series looks good to me. Looking forward to merging v4.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> >  static void __exit dell_wmi_exit(void)
> >  {
> > +   struct calling_interface_buffer *buffer;
> > +
> > +   if (wmi_requires_smbios_request) {
> > +   buffer = dell_smbios_get_buffer();
> > +   buffer->input[0] = 0x1;
> > +   buffer->input[1] = 0x51534554;
> > +   dell_smbios_send_request(17, 3);
> > +   dell_smbios_release_buffer();
> > +   }
> > +
> > wmi_remove_notify_handler(DELL_EVENT_GUID);
> > dell_wmi_input_destroy();
> >  }
> 
> Hi! I would propose moving this get_buffer, send, release code into own
> function with boolean argument on/off. This de-duplicate same code plus
> decrease level of indentation in _init function. And maybe adding ascii
> string comment representation for that 0x5153... number can be useful.

Ah, yes, sure.  I will do that in v4.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

> >  static void __exit dell_wmi_exit(void)
> >  {
> > +   struct calling_interface_buffer *buffer;
> > +
> > +   if (wmi_requires_smbios_request) {
> > +   buffer = dell_smbios_get_buffer();
> > +   buffer->input[0] = 0x1;
> > +   buffer->input[1] = 0x51534554;
> > +   dell_smbios_send_request(17, 3);
> > +   dell_smbios_release_buffer();
> > +   }
> > +
> > wmi_remove_notify_handler(DELL_EVENT_GUID);
> > dell_wmi_input_destroy();
> >  }
> 
> Hi! I would propose moving this get_buffer, send, release code into own
> function with boolean argument on/off. This de-duplicate same code plus
> decrease level of indentation in _init function. And maybe adding ascii
> string comment representation for that 0x5153... number can be useful.

Ah, yes, sure.  I will do that in v4.

-- 
Best regards,
Michał Kępień

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Tuesday 16 February 2016 15:50:28 Michał Kępień wrote:
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + buffer->input[3] = 0x1;
> + dell_smbios_send_request(17, 3);
> + err = buffer->output[0];
> + dell_smbios_release_buffer();
> + if (err) {
> + pr_err("Failed to enable WMI (error %d)\n", err);
> + wmi_remove_notify_handler(DELL_EVENT_GUID);
> + dell_wmi_input_destroy();
> + return dell_smbios_error(err);
> + }
> + }
> +
>   return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> + struct calling_interface_buffer *buffer;
> +
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + dell_smbios_send_request(17, 3);
> + dell_smbios_release_buffer();
> + }
> +
>   wmi_remove_notify_handler(DELL_EVENT_GUID);
>   dell_wmi_input_destroy();
>  }

Hi! I would propose moving this get_buffer, send, release code into own
function with boolean argument on/off. This de-duplicate same code plus
decrease level of indentation in _init function. And maybe adding ascii
string comment representation for that 0x5153... number can be useful.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Pali Rohár]

On Tuesday 16 February 2016 15:50:28 Michał Kępień wrote:
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + buffer->input[3] = 0x1;
> + dell_smbios_send_request(17, 3);
> + err = buffer->output[0];
> + dell_smbios_release_buffer();
> + if (err) {
> + pr_err("Failed to enable WMI (error %d)\n", err);
> + wmi_remove_notify_handler(DELL_EVENT_GUID);
> + dell_wmi_input_destroy();
> + return dell_smbios_error(err);
> + }
> + }
> +
>   return 0;
>  }
>  module_init(dell_wmi_init);
>  
>  static void __exit dell_wmi_exit(void)
>  {
> + struct calling_interface_buffer *buffer;
> +
> + if (wmi_requires_smbios_request) {
> + buffer = dell_smbios_get_buffer();
> + buffer->input[0] = 0x1;
> + buffer->input[1] = 0x51534554;
> + dell_smbios_send_request(17, 3);
> + dell_smbios_release_buffer();
> + }
> +
>   wmi_remove_notify_handler(DELL_EVENT_GUID);
>   dell_wmi_input_destroy();
>  }

Hi! I would propose moving this get_buffer, send, release code into own
function with boolean argument on/off. This de-duplicate same code plus
decrease level of indentation in _init function. And maybe adding ascii
string comment representation for that 0x5153... number can be useful.

-- 
Pali Rohár
pali.ro...@gmail.com

[PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

On some laptop models (e.g. Dell Vostro V131), WMI events are not
generated until a specific SMBIOS request is issued to register an event
listener [1].  As there seems to be no ACPI method or SMBIOS request to
determine without possible side effects whether a given machine needs to
issue this SMBIOS request in order to receive WMI events, DMI matching
is used to whitelist the models which need it.

[1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html

Signed-off-by: Michał Kępień 
---
 drivers/platform/x86/Kconfig|1 +
 drivers/platform/x86/dell-wmi.c |   49 +++
 2 files changed, 50 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3e4d9c3..5ceb53a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -122,6 +122,7 @@ config DELL_WMI
depends on ACPI_WMI
depends on INPUT
depends on ACPI_VIDEO || ACPI_VIDEO = n
+   depends on DELL_SMBIOS
select INPUT_SPARSEKMAP
---help---
  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 368e193..ca8233a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include "dell-smbios.h"
 
 MODULE_AUTHOR("Matthew Garrett ");
 MODULE_AUTHOR("Pali Rohár ");
@@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static u32 dell_wmi_interface_version;
+static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+   wmi_requires_smbios_request = 1;
+   return 1;
+}
+
+static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
+   {
+   .callback = dmi_matched,
+   .ident = "Dell Vostro V131",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+   },
+   },
+   { }
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
@@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
 {
int err;
acpi_status status;
+   struct calling_interface_buffer *buffer;
 
if (!wmi_has_guid(DELL_EVENT_GUID) ||
!wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
@@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
return -ENODEV;
}
 
+   dmi_check_system(dell_wmi_smbios_list);
+
+   if (wmi_requires_smbios_request) {
+   buffer = dell_smbios_get_buffer();
+   buffer->input[0] = 0x1;
+   buffer->input[1] = 0x51534554;
+   buffer->input[3] = 0x1;
+   dell_smbios_send_request(17, 3);
+   err = buffer->output[0];
+   dell_smbios_release_buffer();
+   if (err) {
+   pr_err("Failed to enable WMI (error %d)\n", err);
+   wmi_remove_notify_handler(DELL_EVENT_GUID);
+   dell_wmi_input_destroy();
+   return dell_smbios_error(err);
+   }
+   }
+
return 0;
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
+   struct calling_interface_buffer *buffer;
+
+   if (wmi_requires_smbios_request) {
+   buffer = dell_smbios_get_buffer();
+   buffer->input[0] = 0x1;
+   buffer->input[1] = 0x51534554;
+   dell_smbios_send_request(17, 3);
+   dell_smbios_release_buffer();
+   }
+
wmi_remove_notify_handler(DELL_EVENT_GUID);
dell_wmi_input_destroy();
 }
-- 
1.7.10.4

[PATCH v3 3/5] dell-wmi: enable receiving WMI events on Dell Vostro V131

[Michał Kępień]

On some laptop models (e.g. Dell Vostro V131), WMI events are not
generated until a specific SMBIOS request is issued to register an event
listener [1].  As there seems to be no ACPI method or SMBIOS request to
determine without possible side effects whether a given machine needs to
issue this SMBIOS request in order to receive WMI events, DMI matching
is used to whitelist the models which need it.

[1] https://lists.us.dell.com/pipermail/libsmbios-devel/2015-July/000612.html

Signed-off-by: Michał Kępień 
---
 drivers/platform/x86/Kconfig|1 +
 drivers/platform/x86/dell-wmi.c |   49 +++
 2 files changed, 50 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 3e4d9c3..5ceb53a 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -122,6 +122,7 @@ config DELL_WMI
depends on ACPI_WMI
depends on INPUT
depends on ACPI_VIDEO || ACPI_VIDEO = n
+   depends on DELL_SMBIOS
select INPUT_SPARSEKMAP
---help---
  Say Y here if you want to support WMI-based hotkeys on Dell laptops.
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 368e193..ca8233a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -37,6 +37,7 @@
 #include 
 #include 
 #include 
+#include "dell-smbios.h"
 
 MODULE_AUTHOR("Matthew Garrett ");
 MODULE_AUTHOR("Pali Rohár ");
@@ -47,10 +48,29 @@ MODULE_LICENSE("GPL");
 #define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static u32 dell_wmi_interface_version;
+static bool wmi_requires_smbios_request;
 
 MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
 MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID);
 
+static int __init dmi_matched(const struct dmi_system_id *dmi)
+{
+   wmi_requires_smbios_request = 1;
+   return 1;
+}
+
+static const struct dmi_system_id dell_wmi_smbios_list[] __initconst = {
+   {
+   .callback = dmi_matched,
+   .ident = "Dell Vostro V131",
+   .matches = {
+   DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+   DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+   },
+   },
+   { }
+};
+
 /*
  * Certain keys are flagged as KE_IGNORE. All of these are either
  * notifications (rather than requests for change) or are also sent
@@ -513,6 +533,7 @@ static int __init dell_wmi_init(void)
 {
int err;
acpi_status status;
+   struct calling_interface_buffer *buffer;
 
if (!wmi_has_guid(DELL_EVENT_GUID) ||
!wmi_has_guid(DELL_DESCRIPTOR_GUID)) {
@@ -538,12 +559,40 @@ static int __init dell_wmi_init(void)
return -ENODEV;
}
 
+   dmi_check_system(dell_wmi_smbios_list);
+
+   if (wmi_requires_smbios_request) {
+   buffer = dell_smbios_get_buffer();
+   buffer->input[0] = 0x1;
+   buffer->input[1] = 0x51534554;
+   buffer->input[3] = 0x1;
+   dell_smbios_send_request(17, 3);
+   err = buffer->output[0];
+   dell_smbios_release_buffer();
+   if (err) {
+   pr_err("Failed to enable WMI (error %d)\n", err);
+   wmi_remove_notify_handler(DELL_EVENT_GUID);
+   dell_wmi_input_destroy();
+   return dell_smbios_error(err);
+   }
+   }
+
return 0;
 }
 module_init(dell_wmi_init);
 
 static void __exit dell_wmi_exit(void)
 {
+   struct calling_interface_buffer *buffer;
+
+   if (wmi_requires_smbios_request) {
+   buffer = dell_smbios_get_buffer();
+   buffer->input[0] = 0x1;
+   buffer->input[1] = 0x51534554;
+   dell_smbios_send_request(17, 3);
+   dell_smbios_release_buffer();
+   }
+
wmi_remove_notify_handler(DELL_EVENT_GUID);
dell_wmi_input_destroy();
 }
-- 
1.7.10.4