Hi, On 10/7/22 13:42, Hans de Goede wrote: > Hi, > > On 9/22/22 20:24, Arvid Norlander wrote: >> Hi, >> >> This is version 2 of this patch series, incorporating the various feedback >> on the first version. However, there are some remaining issues that makes >> me keep this marked RFC: >> 1. I tried to get rid of the memory allocation in quickstart_acpi_ghid (as >> suggested by Barnabás Pőcze), but I could not get that working. I'm not >> sure why I did wrong, but I kept getting ACPI errors indicating a buffer >> overflow. I would appreciate knowing how to allocate the buffer on stack >> properly in this case. The memory leak is at least fixed on the error >> path though. > > It can be quite hard to predict how large an object ACPI methods will > return. Even if you get it right for your laptop model it may fail > on other models. So using ACPI_ALLOCATE_BUFFER here (which I assume this > is about) is absolutely fine, I would even say it is a good idea :) > >> 2. The open question mentioned in the original cover letter remains >> undiscussed. I would still like some feedback on those points as well. >> >> The original cover letter follows: >> >> In the following patch series I implement support for three buttons on >> the Toshiba Satellite/Portege Z830 (same laptop, different markets). >> >> These buttons work via a PNP0C32 ACPI device. Hans de Goede pointed out >> an old and flawed attempt to implement this as a staging driver. >> >> With that staging driver as a starting point I have now implemented proper >> support. I believe I have fixed the flaws with the original staging driver. >> As it required almost a complete rewrite I have decided to present it as a >> new driver instead of starting with a revert commit to restore the old >> driver and then apply fixes on top. >> >> The specification for PNP0C32 devices exists as a Microsoft specification. >> It was previously available on their web site, but seems to have been taken >> down during the last month. I had a local copy and I have uploaded it to >> archive.org. It is available here for anyone interested (including a >> conversion of the docx to PDF): >> >> https://archive.org/details/microsoft-acpi-dirapplaunch >> >> The old emails about support for these buttons can be found at: >> https://marc.info/?l=linux-acpi&m=120550727131007 >> https://lkml.org/lkml/2010/5/28/327 >> >> Table of contents: >> 1. Summary of standard >> 2. Issues >> 2.1. Issue 1: Wake support >> 2.2. Issue 2: Button identification >> 2.3. Issue 3: GHID: 64-bit values? >> 2.4. Issue 4: MAINTAINERS? >> 3. User space API >> 3.1. Input device >> 3.2. Sysfs file: button_id (Read only) >> 3.3. Sysfs file: wakeup_cause (Read write) >> 4. HCI_HOTKEY_EVENT register (toshiba_acpi) >> >> >> 1. Summary of standard >> ====================== >> >> Here is a brief high level summary of the standard for PNP0C32. See >> https://archive.org/details/microsoft-acpi-dirapplaunch for the full >> standard. >> >> PNP0C32 devices are "Direct Application Launch" buttons. The idea is that >> they should work while the laptop is in various sleep modes (or even off). >> The Z830 does not support waking from any sleep mode using these buttons, >> it only supports them while it is awake. >> >> Each PNP0C32 device represents a single button. Their meaning is completely >> vendor defined. On Windows you can either: >> * Make them launch an application when pressed (defined in the registry) >> * Or an application can subscribe to specific Window messages to get >> notified when they are pressed (this is how they are used by the Toshiba >> software). >> >> 2. Issues >> ========= >> Unfortunately there are a few issues where I would like some input. >> >> On top of that I'm sure there are lots of issues as I'm fairly new to >> kernel programming! >> >> 2.1. Issue 1: Wake support >> -------------------------- >> This is untested as the Toshiba Z830 that I have simply does not support >> this part in the firmware. I left the old behaviour in and only adapted it >> slightly. >> >> The driver adds a sysfs file "wakeup_cause" to each PNP0C32 device >> (inspired by old approach) that would read "true" after causing the wakeup. >> It would be up to user space query this and reset the value to false. >> This is basically what the old staging driver did, only moved from an >> (un-needed) platform driver to each ACPI driver. >> >> As I cannot test it (the Z830 does not support the wakeup part of the spec) >> I'm more inclined to just drop this feature, especially if the current >> approach is suboptimal. It would then be up to someone else to implement >> this in the future. > > Hmm, since you have already written / ported the wakeup_cause code > I would prefer to retain it. > > You could add a module_param (boolean, default off) to enable this using > a is_visible callback which returns 0 as mode when the boolean is not set > (thus hiding the wakeup_cause sysfs attribute). > Then people can easily test this on other models and if it turns out to > be useful (and works as is) then we can drop the parameter and just > always enable this. > > That is not the prettiest of solutions, but this way we atleast preserve > the work/functionality from the staging driver. So thinking more about this, I believe that the module param would be over kill and I think it is best to just keep this with the suggested changes from the review added. If it works on other models then it might be useful to some users; and if it turns out to not work then we can change/fix it without worrying about breaking existing users of the API since if it does not work in the first place then there won't be any users. Regards, Hans
