Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1712025 > unrolled thread

RE: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time "Zheng, Lv" <lv.zheng@intel.com> - 2017-08-15 12:00 +0200
    Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-08-15 14:30 +0200
      RE: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time "Zheng, Lv" <lv.zheng@intel.com> - 2017-08-17 04:30 +0200

#1712025 — RE: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time

Hi,

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> 
> On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> > Hi, Rafael
> >
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > >
> > > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > > Hi, Rafael
> > > >
> > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > >
> > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > >
> > > > > In some cases GPEs are already active when they are enabled by
> > > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > > on the result of handling the events signaled by them, so the
> > > > > events should not be discarded (which is what happens currently) and
> > > > > they should be handled as soon as reasonably possible.
> > > > >
> > > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > > dispatch GPEs with the status flag set in-band right after
> > > > > enabling them.
> > > >
> > > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > > right after enabling an GPE. So there are 2 conditions related:
> > > > 1. GPE is enabled for the first time.
> > > > 2. GPE is initialized.
> > > >
> > > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > > all GPE EN bits are actually disabled.
> > >
> > > But we don't do it today, do we?
> >
> > We don't do that.
> >
> > >
> > > And still calling _dispatch() should not be incorrect even if the GPE
> > > has been enabled already at this point.  Worst case it just will
> > > queue up the execution of _Lxx/_Exx which may or may not do anything
> > > useful.
> > >
> > > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > > will block on it if run concurrently and we've checked the status, so
> > > we know that the GPE *should* be dispatched, so I sort of fail to see
> > > the problem.
> >
> > There is another problem related:
> > ACPICA clears GPEs before enabling it.
> > This is proven to be wrong, and we have to fix it:
> > https://bugzilla.kernel.org/show_bug.cgi?id=196249
> >
> > without fixing this issue, in this solution, we surely need to save the
> > GPE STS bit before incrementing GPE reference count, and poll it according
> > to the saved STS bit. Because if we poll it after enabling, STS bit will
> > be wrongly cleared.
> 
> I'm not sure if I understand you correctly, but why would we poll it?
> 
> In the $subject patch the status is checked and then
> acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
> 
> If this is the first reference (which will be the case in the majority
> of cases), acpi_ev_enable_gpe() will be called and that will clear the
> status.
> 
> Then, acpi_ev_gpe_dispatch() is called if the status was set and that
> itself doesn't check the status.  It disables the GPE upfront (so the
> status doesn't matter from now on until the GPE is enabled again) and
> clears the status unconditionally if the GPE is edge-triggered.  This
> means that for edge-triggered GPEs the clearing of the status by
> acpi_ev_enable_gpe() doesn't matter here.

No problem, I understood.
And was thinking thought this patch [edge GPE dispatch fix] should be
correct and good for current Linux upstream.

What I meant is:
PATCH [edge GPE clear fix] https://patchwork.kernel.org/patch/9894983/
is a fix we need for upstream as it is the only possible fix for the
issue fixed by it.
On top of that, when acpi_ev_enable_gpe() is called, GPE won't be cleared.
and then things can be done in a simpler way:
PATCH [edge GPE enable fix] https://patchwork.kernel.org/patch/9894989/

As [edge GPE clear fix] is risky, I think [edge GPE dispatch fix] is OK
for Linux upstream.

So we can have 2 processes:
1. Merge [edge GPE dispatch fix] and let [edge GPE clear fix] and
   [edge GPE enable fix] released from ACPICA upstream so that:
   1. We can enhance them in ACPICA upstream.
   2. It will be regression safer for us to merge [edge GPE clear fix].
2. Merge [edge GPE clear fix] and [edge GPE enable fix] without
   merging [edge GPE dispatch fix].

What I meant is:
It's up to you to decide which process we should take. :)

> 
> For level-triggered GPEs the status is not cleared by acpi_ev_gpe_dispatch()
> itself, but _Lxx is executed (again, without checking the status) and
> finally the status is cleared by acpi_ev_finish_gpe() anyway, so we
> end up with cleared status no matter what (and that before re-enabling
> the GPE).  End even if _Lxx itself checked the status (but honestly why
> would it?), then this is a level-triggered GPE, so it will re-trigger
> anyway if not handled this time.

Let's skip level-triggered GPE case.

> 
> So it looks like the fact that acpi_ev_enable_gpe() clears the status before
> enabling the GPE doesn't matter for this patch, although it may matter in
> general, but that is a different (and somewhat related) issue.

I agreed.

Thanks and best regards
Lv

[toc] | [next] | [standalone]

#1712134

On Tuesday, August 15, 2017 11:59:00 AM CEST Zheng, Lv wrote:
> Hi,
> 
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > 
> > On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> > > Hi, Rafael
> > >
> > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > >
> > > > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > > > Hi, Rafael
> > > > >
> > > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > > >
> > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > >
> > > > > > In some cases GPEs are already active when they are enabled by
> > > > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > > > on the result of handling the events signaled by them, so the
> > > > > > events should not be discarded (which is what happens currently) and
> > > > > > they should be handled as soon as reasonably possible.
> > > > > >
> > > > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > > > dispatch GPEs with the status flag set in-band right after
> > > > > > enabling them.
> > > > >
> > > > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > > > right after enabling an GPE. So there are 2 conditions related:
> > > > > 1. GPE is enabled for the first time.
> > > > > 2. GPE is initialized.
> > > > >
> > > > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > > > all GPE EN bits are actually disabled.
> > > >
> > > > But we don't do it today, do we?
> > >
> > > We don't do that.
> > >
> > > >
> > > > And still calling _dispatch() should not be incorrect even if the GPE
> > > > has been enabled already at this point.  Worst case it just will
> > > > queue up the execution of _Lxx/_Exx which may or may not do anything
> > > > useful.
> > > >
> > > > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > > > will block on it if run concurrently and we've checked the status, so
> > > > we know that the GPE *should* be dispatched, so I sort of fail to see
> > > > the problem.
> > >
> > > There is another problem related:
> > > ACPICA clears GPEs before enabling it.
> > > This is proven to be wrong, and we have to fix it:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=196249
> > >
> > > without fixing this issue, in this solution, we surely need to save the
> > > GPE STS bit before incrementing GPE reference count, and poll it according
> > > to the saved STS bit. Because if we poll it after enabling, STS bit will
> > > be wrongly cleared.
> > 
> > I'm not sure if I understand you correctly, but why would we poll it?
> > 
> > In the $subject patch the status is checked and then
> > acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
> > 
> > If this is the first reference (which will be the case in the majority
> > of cases), acpi_ev_enable_gpe() will be called and that will clear the
> > status.
> > 
> > Then, acpi_ev_gpe_dispatch() is called if the status was set and that
> > itself doesn't check the status.  It disables the GPE upfront (so the
> > status doesn't matter from now on until the GPE is enabled again) and
> > clears the status unconditionally if the GPE is edge-triggered.  This
> > means that for edge-triggered GPEs the clearing of the status by
> > acpi_ev_enable_gpe() doesn't matter here.
> 
> No problem, I understood.
> And was thinking thought this patch [edge GPE dispatch fix] should be
> correct and good for current Linux upstream.
> 
> What I meant is:
> PATCH [edge GPE clear fix] https://patchwork.kernel.org/patch/9894983/
> is a fix we need for upstream as it is the only possible fix for the
> issue fixed by it.
> On top of that, when acpi_ev_enable_gpe() is called, GPE won't be cleared.
> and then things can be done in a simpler way:
> PATCH [edge GPE enable fix] https://patchwork.kernel.org/patch/9894989/
> 
> As [edge GPE clear fix] is risky, I think [edge GPE dispatch fix] is OK
> for Linux upstream.
> 
> So we can have 2 processes:
> 1. Merge [edge GPE dispatch fix] and let [edge GPE clear fix] and
>    [edge GPE enable fix] released from ACPICA upstream so that:
>    1. We can enhance them in ACPICA upstream.
>    2. It will be regression safer for us to merge [edge GPE clear fix].
> 2. Merge [edge GPE clear fix] and [edge GPE enable fix] without
>    merging [edge GPE dispatch fix].
> 
> What I meant is:
> It's up to you to decide which process we should take. :)

OK

So I would go with what is already there in my tree.  Please work on top of
that.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1713480

Hi,

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Rafael J.
> Wysocki
> Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> 
> On Tuesday, August 15, 2017 11:59:00 AM CEST Zheng, Lv wrote:
> > Hi,
> >
> > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > >
> > > On Friday, August 11, 2017 7:40:56 AM CEST Zheng, Lv wrote:
> > > > Hi, Rafael
> > > >
> > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > Subject: Re: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > >
> > > > > On Thursday, August 10, 2017 3:48:58 AM CEST Zheng, Lv wrote:
> > > > > > Hi, Rafael
> > > > > >
> > > > > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > > > > Subject: [PATCH 1/3] ACPICA: Dispatch active GPEs at init time
> > > > > > >
> > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > > >
> > > > > > > In some cases GPEs are already active when they are enabled by
> > > > > > > acpi_ev_initialize_gpe_block() and whatever happens next may depend
> > > > > > > on the result of handling the events signaled by them, so the
> > > > > > > events should not be discarded (which is what happens currently) and
> > > > > > > they should be handled as soon as reasonably possible.
> > > > > > >
> > > > > > > For this reason, modify acpi_ev_initialize_gpe_block() to
> > > > > > > dispatch GPEs with the status flag set in-band right after
> > > > > > > enabling them.
> > > > > >
> > > > > > In fact, what we need seems to be invoking acpi_ev_gpe_dispatch()
> > > > > > right after enabling an GPE. So there are 2 conditions related:
> > > > > > 1. GPE is enabled for the first time.
> > > > > > 2. GPE is initialized.
> > > > > >
> > > > > > And we need to make sure that before acpi_update_all_gpes() is invoked,
> > > > > > all GPE EN bits are actually disabled.
> > > > >
> > > > > But we don't do it today, do we?
> > > >
> > > > We don't do that.
> > > >
> > > > >
> > > > > And still calling _dispatch() should not be incorrect even if the GPE
> > > > > has been enabled already at this point.  Worst case it just will
> > > > > queue up the execution of _Lxx/_Exx which may or may not do anything
> > > > > useful.
> > > > >
> > > > > And BTW this is all done under acpi_gbl_gpe_lock so acpi_ev_gpe_detect()
> > > > > will block on it if run concurrently and we've checked the status, so
> > > > > we know that the GPE *should* be dispatched, so I sort of fail to see
> > > > > the problem.
> > > >
> > > > There is another problem related:
> > > > ACPICA clears GPEs before enabling it.
> > > > This is proven to be wrong, and we have to fix it:
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=196249
> > > >
> > > > without fixing this issue, in this solution, we surely need to save the
> > > > GPE STS bit before incrementing GPE reference count, and poll it according
> > > > to the saved STS bit. Because if we poll it after enabling, STS bit will
> > > > be wrongly cleared.
> > >
> > > I'm not sure if I understand you correctly, but why would we poll it?
> > >
> > > In the $subject patch the status is checked and then
> > > acpi_ev_add_gpe_reference() is called to add a reference to the GPE.
> > >
> > > If this is the first reference (which will be the case in the majority
> > > of cases), acpi_ev_enable_gpe() will be called and that will clear the
> > > status.
> > >
> > > Then, acpi_ev_gpe_dispatch() is called if the status was set and that
> > > itself doesn't check the status.  It disables the GPE upfront (so the
> > > status doesn't matter from now on until the GPE is enabled again) and
> > > clears the status unconditionally if the GPE is edge-triggered.  This
> > > means that for edge-triggered GPEs the clearing of the status by
> > > acpi_ev_enable_gpe() doesn't matter here.
> >
> > No problem, I understood.
> > And was thinking thought this patch [edge GPE dispatch fix] should be
> > correct and good for current Linux upstream.
> >
> > What I meant is:
> > PATCH [edge GPE clear fix] https://patchwork.kernel.org/patch/9894983/
> > is a fix we need for upstream as it is the only possible fix for the
> > issue fixed by it.
> > On top of that, when acpi_ev_enable_gpe() is called, GPE won't be cleared.
> > and then things can be done in a simpler way:
> > PATCH [edge GPE enable fix] https://patchwork.kernel.org/patch/9894989/
> >
> > As [edge GPE clear fix] is risky, I think [edge GPE dispatch fix] is OK
> > for Linux upstream.
> >
> > So we can have 2 processes:
> > 1. Merge [edge GPE dispatch fix] and let [edge GPE clear fix] and
> >    [edge GPE enable fix] released from ACPICA upstream so that:
> >    1. We can enhance them in ACPICA upstream.
> >    2. It will be regression safer for us to merge [edge GPE clear fix].
> > 2. Merge [edge GPE clear fix] and [edge GPE enable fix] without
> >    merging [edge GPE dispatch fix].
> >
> > What I meant is:
> > It's up to you to decide which process we should take. :)
> 
> OK
> 
> So I would go with what is already there in my tree.  Please work on top of
> that.

OK.
Then I'll do that from ACPICA upstream to have more eyes on the enhancements
and more time to create the stable baseline for the [edge GPE clear fix].

Best regards,
Lv

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web