Re: [PATCH RFC v2 0/2] Quickstart buttons driver and Toshiba Z830

[Hans de Goede <hdegoede@redhat.com>]

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