Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
On Wed, Dec 30, 2015 at 3:27 AM, Michał Kępieńwrote: >> > If you feel like I'm nit-picking and none of the above matters, >> > please feel free to disregard my input and just follow your gut. >> >> It's ok. We just understand it quite differently. And in this case what >> about changing commit message to something like this? >> >> === >> dell-wmi: Check if Dell WMI descriptor structure is valid >> >> After examining existing DSDT ACPI tables of more laptops and looking >> into Dell WMI document mentioned in ML dicussion archived at >> http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will >> parse and check WMI descriptor if contains expected data. It is because >> WMI descriptor contains interface version number and it is needed to >> know in next commit. >> === > > I like it way more than the previous one. > FWIW, the original version of this series works fine on my laptop (after fixing the separate dmi_walk misuse bug). --Andy -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
> > If you feel like I'm nit-picking and none of the above matters, > > please feel free to disregard my input and just follow your gut. > > It's ok. We just understand it quite differently. And in this case what > about changing commit message to something like this? > > === > dell-wmi: Check if Dell WMI descriptor structure is valid > > After examining existing DSDT ACPI tables of more laptops and looking > into Dell WMI document mentioned in ML dicussion archived at > http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will > parse and check WMI descriptor if contains expected data. It is because > WMI descriptor contains interface version number and it is needed to > know in next commit. > === I like it way more than the previous one. -- Best regards, Michał Kępień -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
On Tuesday 29 December 2015 13:44:13 Michał Kępień wrote: > > > > According to Dell WMI document mentioned in ML dicussion > > > > archived at > > > > http://www.spinics.net/lists/platform-driver-x86/msg07220.html > > > > OS should check Dell WMI descriptor structure. > > > > > > "Should" or "can"? I skimmed through the ACPI-WMI PDF and > > > Mario's message again and I couldn't find any explicit statement > > > urging the reader to check the structure in question before > > > doing anything else. > > > > That's questionable... In "Design flow" is first point that WMI > > descriptor check. > > Which "Design flow" are you referring to? Because I found at least > two: chapter 2.3 and a subsection of chapter 2.3.3. Funnily enough, > in both of these locations the WMI Descriptor Method is discussed > first. > > Personally, I wouldn't use the structure of that document to draw > cause-effect conclusions. Just look at the last chapter (2.3.4), > which shows how to tell whether the BIOS supports the ACPI-WMI > interface. Shouldn't that be the first thing to check, before doing > anything else mentioned in that document? Yet, it's the last thing > discussed. > > Anyway, while the document mentions in several places that the BIOS > WMI Descriptor object can be queried, it fails to convince me as to > why this is necessary at all as all values in the returned buffer > are constant. Perhaps parsing the buffer is useful as a sanity check > of some kind, but it certainly isn't a prerequisite for performing > further actions. > > Given the nature of your patchset, I'd personally rephrase the commit > message(s) to state that according to your observations, there are > behavioral differences between models with different versions of the > WMI Interface, so we parse the WMI Descriptor object to determine > which WMI Interface version is used on the machine we're running on. > Perhaps with an additional word or two that it won't hurt to also > check the WMI Descriptor object's correctness while we're at it. > > If you feel like I'm nit-picking and none of the above matters, > please feel free to disregard my input and just follow your gut. It's ok. We just understand it quite differently. And in this case what about changing commit message to something like this? === dell-wmi: Check if Dell WMI descriptor structure is valid After examining existing DSDT ACPI tables of more laptops and looking into Dell WMI document mentioned in ML dicussion archived at http://www.spinics.net/lists/platform-driver-x86/msg07220.html we will parse and check WMI descriptor if contains expected data. It is because WMI descriptor contains interface version number and it is needed to know in next commit. === -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
> > > According to Dell WMI document mentioned in ML dicussion archived > > > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html > > > OS should check Dell WMI descriptor structure. > > > > "Should" or "can"? I skimmed through the ACPI-WMI PDF and Mario's > > message again and I couldn't find any explicit statement urging the > > reader to check the structure in question before doing anything else. > > That's questionable... In "Design flow" is first point that WMI > descriptor check. Which "Design flow" are you referring to? Because I found at least two: chapter 2.3 and a subsection of chapter 2.3.3. Funnily enough, in both of these locations the WMI Descriptor Method is discussed first. Personally, I wouldn't use the structure of that document to draw cause-effect conclusions. Just look at the last chapter (2.3.4), which shows how to tell whether the BIOS supports the ACPI-WMI interface. Shouldn't that be the first thing to check, before doing anything else mentioned in that document? Yet, it's the last thing discussed. Anyway, while the document mentions in several places that the BIOS WMI Descriptor object can be queried, it fails to convince me as to why this is necessary at all as all values in the returned buffer are constant. Perhaps parsing the buffer is useful as a sanity check of some kind, but it certainly isn't a prerequisite for performing further actions. Given the nature of your patchset, I'd personally rephrase the commit message(s) to state that according to your observations, there are behavioral differences between models with different versions of the WMI Interface, so we parse the WMI Descriptor object to determine which WMI Interface version is used on the machine we're running on. Perhaps with an additional word or two that it won't hurt to also check the WMI Descriptor object's correctness while we're at it. If you feel like I'm nit-picking and none of the above matters, please feel free to disregard my input and just follow your gut. -- Best regards, Michał Kępień -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
> According to Dell WMI document mentioned in ML dicussion archived at > http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should > check Dell WMI descriptor structure. "Should" or "can"? I skimmed through the ACPI-WMI PDF and Mario's message again and I couldn't find any explicit statement urging the reader to check the structure in question before doing anything else. -- Best regards, Michał Kępień -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
On Monday 28 December 2015 14:37:07 Michał Kępień wrote: > > According to Dell WMI document mentioned in ML dicussion archived > > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html > > OS should check Dell WMI descriptor structure. > > "Should" or "can"? I skimmed through the ACPI-WMI PDF and Mario's > message again and I couldn't find any explicit statement urging the > reader to check the structure in question before doing anything else. That's questionable... In "Design flow" is first point that WMI descriptor check. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
On Friday 25 December 2015 02:23:04 Andy Lutomirski wrote: > On Thu, Dec 24, 2015 at 1:18 PM, Pali Rohár> wrote: > > According to Dell WMI document mentioned in ML dicussion archived > > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html > > OS should check Dell WMI descriptor structure. Structure also > > provide Dell WMI interface version which is used later. > > I will rebase my big series on top of this. It'll give me a good > excuse to test that I got the probe ordering right. (The code is > explicitly intended to support use cases like this, and now I'll have > a real-world test for it.) I'll also test this in a bit. Ok! > > +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID); > > I don't think this is necessary. The driver will only work if both > wmi devices and, hence, modaliases are present, so there's no need to > cause just one or the other to trigger dell-wmi autoloading. Maybe now when you are working on big WMI patch series is time to change modalias support in WMI to support AND-conjunction (&&) on WMI aliases, not just OR (one alias match). Something like: load dell-wmi.ko driver if system provides both WMI GUIDs. > > +/** > > > + * Descriptor buffer is 128 byte long and contains: > This isn't kerneldoc format, so I think this should just be "/*". > Ok, I will fix this in next version. > > + if (obj->buffer.length != 128) { > > + pr_err("Dell descriptor buffer has invalid length > > (%d)\n", + obj->buffer.length); > > + kfree(obj); > > + return -EINVAL; > > + } > > I would advocate for being more permissive: a buffer that is actually > too short for the fields we need would result in -EINVAL, but a > buffer that isn't 128 bytes would just be a warning and not cause > module load to fail. > > --Andy Sounds good, I will change this part. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: This is a digitally signed message part.
Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
On Thu, Dec 24, 2015 at 1:18 PM, Pali Rohárwrote: > According to Dell WMI document mentioned in ML dicussion archived at > http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should > check Dell WMI descriptor structure. Structure also provide Dell WMI > interface version which is used later. I will rebase my big series on top of this. It'll give me a good excuse to test that I got the probe ordering right. (The code is explicitly intended to support use cases like this, and now I'll have a real-world test for it.) I'll also test this in a bit. > +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID); I don't think this is necessary. The driver will only work if both wmi devices and, hence, modaliases are present, so there's no need to cause just one or the other to trigger dell-wmi autoloading. > +/** > + * Descriptor buffer is 128 byte long and contains: This isn't kerneldoc format, so I think this should just be "/*". > + if (obj->buffer.length != 128) { > + pr_err("Dell descriptor buffer has invalid length (%d)\n", > + obj->buffer.length); > + kfree(obj); > + return -EINVAL; > + } I would advocate for being more permissive: a buffer that is actually too short for the fields we need would result in -EINVAL, but a buffer that isn't 128 bytes would just be a warning and not cause module load to fail. --Andy -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid
According to Dell WMI document mentioned in ML dicussion archived at http://www.spinics.net/lists/platform-driver-x86/msg07220.html OS should check Dell WMI descriptor structure. Structure also provide Dell WMI interface version which is used later. Signed-off-by: Pali Rohár--- drivers/platform/x86/dell-wmi.c | 78 ++- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c index 57402c4..09ee8ed 100644 --- a/drivers/platform/x86/dell-wmi.c +++ b/drivers/platform/x86/dell-wmi.c @@ -2,6 +2,7 @@ * Dell WMI hotkeys * * Copyright (C) 2008 Red Hat + * Copyright (C) 2014-2015 Pali Rohár * * Portions based on wistron_btns.c: * Copyright (C) 2005 Miloslav Trmac @@ -38,14 +39,18 @@ #include MODULE_AUTHOR("Matthew Garrett "); +MODULE_AUTHOR("Pali Rohár "); MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver"); MODULE_LICENSE("GPL"); #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492" +#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492" static int acpi_video; +static u32 dell_wmi_interface_version; MODULE_ALIAS("wmi:"DELL_EVENT_GUID); +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID); /* * Certain keys are flagged as KE_IGNORE. All of these are either @@ -422,16 +427,85 @@ static void __init find_hk_type(const struct dmi_header *dm, void *dummy) } } +/** + * Descriptor buffer is 128 byte long and contains: + * + * Name Offset Length Value + * Vendor Signature 0 4"DELL" + * Object Signature 4 4" WMI" + * WMI Interface Version 8 4 + * WMI buffer length12 44096 + */ +static int __init dell_wmi_check_descriptor_buffer(void) +{ + struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL }; + union acpi_object *obj; + acpi_status status; + u32 *buffer; + + status = wmi_query_block(DELL_DESCRIPTOR_GUID, 0, ); + if (ACPI_FAILURE(status)) { + pr_err("Cannot read Dell descriptor buffer - %d\n", status); + return status; + } + + obj = (union acpi_object *)out.pointer; + if (!obj) { + pr_err("Dell descriptor buffer is empty\n"); + return -EINVAL; + } + + if (obj->type != ACPI_TYPE_BUFFER) { + pr_err("Cannot read Dell descriptor buffer\n"); + kfree(obj); + return -EINVAL; + } + + if (obj->buffer.length != 128) { + pr_err("Dell descriptor buffer has invalid length (%d)\n", + obj->buffer.length); + kfree(obj); + return -EINVAL; + } + + buffer = (u32 *)obj->buffer.pointer; + + if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720) + pr_warn("Dell descriptor buffer has invalid signature (%*ph)\n", + 8, buffer); + + if (buffer[2] != 0 && buffer[2] != 1) + pr_warn("Dell descriptor buffer has unknown version (%d)\n", + buffer[2]); + + if (buffer[3] != 4096) + pr_warn("Dell descriptor buffer has invalid buffer length (%d)\n", + buffer[3]); + + dell_wmi_interface_version = buffer[2]; + + pr_info("Detected Dell WMI interface version %u\n", + dell_wmi_interface_version); + + kfree(obj); + return 0; +} + static int __init dell_wmi_init(void) { int err; acpi_status status; - if (!wmi_has_guid(DELL_EVENT_GUID)) { - pr_warn("No known WMI GUID found\n"); + if (!wmi_has_guid(DELL_EVENT_GUID) || + !wmi_has_guid(DELL_DESCRIPTOR_GUID)) { + pr_warn("Dell WMI GUID were not found\n"); return -ENODEV; } + err = dell_wmi_check_descriptor_buffer(); + if (err) + return err; + dmi_walk(find_hk_type, NULL); acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor; -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html