On 13-01-2020 11:41, Hans de Goede wrote:
Hi,
On 13-01-2020 04:20, Samuel Holland wrote:
This driver attempts to avoid reporting wakeup events to userspace by
clearing a possible pending IRQ before IRQs are enabled during resume.
The assumption seems to be that userspace cannot cope with a KEY_POWER
press during resume. However, no other input driver does this, so it
would be a bug that such events are missing with this driver.
Furthermore, for PMICs connected via I2C or RSB, it is not possible to
update the regmap during the noirq resume phase, because the bus
controller drivers require IRQs to perform bus transactions. And the
resume hook cannot move to a later phase, because then it would race
with the power key IRQ handler.
So the best solution seems to be simply removing the hook.
Hmm, I'm not sure this is a good idea, let me give you some background
info on this:
This hook was handled because on X86 systems/laptops when waking
them up typically the power-button does not send a KEY_POWER press event
when the system was woken up through the power-button.
So normal (e.g. Debian, Fedora) userspace does not expect this event
and will directly go to sleep again because that is the default behavior
on a KEY_POWER event.
p.s.
The main reason why typical userspace does not expect the KEY_POWER
event is because most of the devices which run mainline and have
suspend/resume working and are using the ACPI button driver for
the power-button: drivers/acpi/button.c, which has:
acpi_pm_wakeup_event(&device->dev);
if (button->suspended)
break;
keycode = test_bit(KEY_SLEEP, input->keybit) ?
KEY_SLEEP : KEY_POWER;
input_report_key(input, keycode, 1);
input_sync(input);
input_report_key(input, keycode, 0);
input_sync(input);
And:
static int acpi_button_suspend(struct device *dev)
{
struct acpi_device *device = to_acpi_device(dev);
struct acpi_button *button = acpi_driver_data(device);
button->suspended = true;
return 0;
}
static int acpi_button_resume(struct device *dev)
{
struct acpi_device *device = to_acpi_device(dev);
struct acpi_button *button = acpi_driver_data(device);
button->suspended = false;
return 0;
}
So when the ACPI notify for the button runs on resume, suspended is
still true; and no KEY_POWER event is send...
Arguably to be consistent we should fix drivers/acpi/button.c to
send KEY_POWER on wakeup too, but that will break things for many
users with a likelyhood of breakage approaching 100%.
Note I'm not telling this to argue against your change, just as
background for why I added this behavior to the axp20x-pek code.
Oh and taking a second look, I see that the hook is already
written so as to only execute on the AXP288 PMIC, which means
it should effectively already only influence X86 machines.
So are you trying to get the KEY_POWER event after wakeup
by power-button to work on a X86 device ? In that case please
be aware of the drivers/acpi/button.c issue...
I have the feeling that we may need a Kconfig option to configure
whether or not to send KEY_POWER on wakeup by power-button, because
as discussed in my previous mail Android wants this, KDE/MATE/GNOME
not so much...
Regards,
Hans
On x86 axp20x-pek is only used for the power-button on Bay Trail devices
with a AXP288 PMIC. On Cherry Trail devices with an AXP288 PMIC the
power-button is also connected directly to a GPIO on the SoC and that
is used (also see the axp20x_pek_should_register_input function).
So after writing this patch, when doing hw-enablement for the power-button
on the Cherry Trail devices I learned that the gpio_keys driver does
send userspace a KEY_POWER event when woken up with the power-button.
I wrote a patch for gpio-keys to not do this, as that is what normal
Linux userspace expects, but that was nacked, because under e.g.
Android the KEY_POWER event is actually desirable / necessary to avoid
Android immediately re-suspending the system again. Since my "fix" to
the gpio-keys devices was nacked I have instead wroked around this in
userspace, but *only* for the GNOME3 desktop environment, by teaching
GNOME3 to ignore KEY_POWER events for the first couple of seconds after
a resume.
So your suggested change, which will cause KEY_POWER to be send on
Bay Trail devices after a wake-up by the power button, should be
fine for recent GNOME3 versions, but for other desktop environments
this may cause a regression where they respond to the new KEY_POWER
event by immediately going back to sleep again.
As for this not working with the i2c bus, it does on X86 because
the PMIC is also directly accessed by the power-management HW of
the SoC and to make this work the i2c-controller is never suspended
and its irq is marked IRQF_NO_SUSPEND. But this is X86 special
sauce.
Summarizing:
I'm personally fine with remove the magic I added to suppress
the KEY_POWER press reporting as in hindsight given the gpio-keys
story I should have never added it. But I'm worried about this
causing regressions for some Bay Trail users. OTOH making this
change would be good for Android X86 users.
Another IMHO better fix would be to drop the __maybe_unused and
instead wrap both the axp20x_pek_resume_noirq function and the
init of the .resume_noirq struct member with:
#if defined X86 && defined CONFIG_PM_SLEEP
This keeps the current behavior on Bay Trail machines, while
I assume it should also fix the issues this was causing for
your setup.
Regards,
Hans
Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
---
drivers/input/misc/axp20x-pek.c | 25 -------------------------
1 file changed, 25 deletions(-)
diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 17c1cca74498..7d0ee5bececb 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -352,30 +352,6 @@ static int axp20x_pek_probe(struct platform_device *pdev)
return 0;
}
-static int __maybe_unused axp20x_pek_resume_noirq(struct device *dev)
-{
- struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
-
- if (axp20x_pek->axp20x->variant != AXP288_ID)
- return 0;
-
- /*
- * Clear interrupts from button presses during suspend, to avoid
- * a wakeup power-button press getting reported to userspace.
- */
- regmap_write(axp20x_pek->axp20x->regmap,
- AXP20X_IRQ1_STATE + AXP288_IRQ_POKN / 8,
- BIT(AXP288_IRQ_POKN % 8));
-
- return 0;
-}
-
-static const struct dev_pm_ops axp20x_pek_pm_ops = {
-#ifdef CONFIG_PM_SLEEP
- .resume_noirq = axp20x_pek_resume_noirq,
-#endif
-};
-
static const struct platform_device_id axp_pek_id_match[] = {
{
.name = "axp20x-pek",
@@ -394,7 +370,6 @@ static struct platform_driver axp20x_pek_driver = {
.id_table = axp_pek_id_match,
.driver = {
.name = "axp20x-pek",
- .pm = &axp20x_pek_pm_ops,
.dev_groups = axp20x_groups,
},
};
