LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ravi Chandra Sadineni <ravisadineni@google.com>
To: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Benson Leung <bleung@google.com>,
	Ravi Chandra Sadineni <ravisadineni@chromium.org>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	Len Brown <lenb@kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Todd Broch <tbroch@google.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	Rajat Jain <rajatja@google.com>,
	Furquan Shaikh <furquan@chromium.org>,
	Benson Leung <bleung@chromium.org>
Subject: Re: [PATCH] ACPI LID: increment wakeup count only when notified.
Date: Mon, 11 Jun 2018 10:59:03 -0700	[thread overview]
Message-ID: <CAOGSYL0dCWZvA1TYTFMWwJ7QGSSmwQcWxmJ+JZerxxt6=vx9Sg@mail.gmail.com> (raw)
In-Reply-To: <CAJZ5v0gXqyKQ5yTtqFYzv=KW2o+b4vGiUYiWWn+iT=zY-mU9xw@mail.gmail.com>

Hi Rafael,

Hopefully this will clear things a bit.

1. Why is this patch needed ?
 Consider the following scenario.
      1. User left the device idle for some time.
      2. A deamon in the userland that controls suspend policy might
suspend the device. The lid is still open.
      3. Now the user returns and wakes the system up by pressing a
key. The system resumes.
      4. In the current implementation we call pm_wakeup_event() (if
the lid is open) irrespective of whether we have received a notify
signal.
      5. The deamon in the userland tries to identify what might woken
the system up based on the wakeup_count.
      6. The deamon sees a wakeup_count  increment for both keyboard
and lid and is confused.
 This patch is an attempt to address the same.

2. Will it break any existing logic ?
   AFAIK pm_wakeup_event() serves 2 purposes.
               1. Helps preventing a race between system wide suspend
and wakeup event.
               2. Helps identifying the device that woke the system up.

As far as preventing races during suspend is concerned, in the
existing implementation, we increment the wakeup_count only when there
is a lid open. Thus only lid open can prevent the system from
suspending (if lid is a wake enabled device). But with my previous
change, we will end up incrementing the wakeup_count for both lid
close and lid open. Fixed this in V2,  so that we do not  increment
the wakeup_count when there is a lid close.

And currently we cannot identify if the lid is the reason for wake
anyway.  So this patch can only make things better here.

Thanks,
Ravi

On Wed, Jun 6, 2018 at 4:21 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Thu, Jun 7, 2018 at 1:11 AM, Benson Leung <bleung@google.com> wrote:
>> Hi Rafael,
>>
>> On Wed, Jun 06, 2018 at 09:00:43AM +0200, Rafael J. Wysocki wrote:
>>> > @@ -417,6 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event)
>>> >                 /* fall through */
>>> >         case ACPI_BUTTON_NOTIFY_STATUS:
>>> >                 input = button->input;
>>> > +               acpi_pm_wakeup_event(&device->dev);
>>>
>>> Not really.
>>>
>>> There already is an acpi_pm_wakeup_event() call in the else branch below.
>>>
>>
>> Ravi removes that other call below.
>
> OK, I missed that, not sure why, sorry.
>
>> The intent for this is to call
>> acpi_pm_wakeup_event() regardless if the button->type is ACPI_BUTTON_TYPE_LID,
>> in case that event is ACPI_BUTTON_NOTIFY_STATUS.
>
> Well, the patch really drops the acpi_pm_wakeup_event() call from
> acpi_lid_notify_state() and so it has to ensure that this function
> will be called here for ACPI_BUTTON_NOTIFY_STATUS regardless of the
> button->type value.
>
> That's fine, but still the changelog doesn't make it clear why the
> acpi_pm_wakeup_event() call in acpi_lid_notify_state() is not
> necessary in general.
>
> Thanks,
> Rafael

      parent reply	other threads:[~2018-06-11 17:59 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-04 18:26 Ravi Chandra Sadineni
2018-06-04 19:19 ` Benson Leung
2018-06-06  7:00 ` Rafael J. Wysocki
2018-06-06 23:11   ` Benson Leung
2018-06-06 23:21     ` Rafael J. Wysocki
2018-06-11 17:57       ` [PATCH V2] " Ravi Chandra Sadineni
     [not found]         ` <CAOGSYL371YSqpqdzqHUC+UxvMtxTc0q=YFPRcT-SeSHO5Pepeg@mail.gmail.com>
2018-06-21 13:11           ` Rafael J. Wysocki
2018-06-26  9:55         ` Rafael J. Wysocki
2018-06-27 17:55           ` [PATCH V3] " Ravi Chandra Sadineni
2018-07-04 10:39             ` Rafael J. Wysocki
2018-06-11 17:59       ` Ravi Chandra Sadineni [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOGSYL0dCWZvA1TYTFMWwJ7QGSSmwQcWxmJ+JZerxxt6=vx9Sg@mail.gmail.com' \
    --to=ravisadineni@google.com \
    --cc=bleung@chromium.org \
    --cc=bleung@google.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=furquan@chromium.org \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=rajatja@google.com \
    --cc=ravisadineni@chromium.org \
    --cc=rjw@rjwysocki.net \
    --cc=tbroch@google.com \
    --subject='Re: [PATCH] ACPI LID: increment wakeup count only when notified.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).