* [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests
@ 2011-09-22 23:16 Prarit Bhargava
2011-09-23 1:08 ` Bjorn Helgaas
0 siblings, 1 reply; 6+ messages in thread
From: Prarit Bhargava @ 2011-09-22 23:16 UTC (permalink / raw)
To: linux-acpi; +Cc: Prarit Bhargava, mjg
This patch resolves a process hang in which a PCI card with a PCI-to-PCI
bridge is removed via the acpiphp driver.
The issue is that during the remove of the card an ACPI event occurs
which is erroneously added the kacpi_notify_wq queue. This add is done in
acpi_ev_queue_notify_request() via a call to acpi_os_execute().
Eventually, the event runs on the kacpi_notify_wq and the code attempts to
remove the card. During the hotplug remove of the device, the following
call sequence happens
cleanup_p2p_bridge()
-> cleanup_bridge()
-> acpi_remove_notify_handler()
-> acpi_os_wait_events_complete()
-> flush_workqueue(kacpi_notify_wq)
which is the queue we are currently executing on and the process will hang.
The problem is that the event is placed on the wrong queue. The code already
contains a kacpi_hotplug_wq queue for hotplug events to be added to in order
to prevent hangs like this.
In acpi_ev_queue_notify_request() the code should check to see if this is a
Ejection Request and if it is, add it the kacpi_hotplug_wq via a call to
acpi_os_hotplug_execute().
Cc: mjg@redhat.com
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
---
drivers/acpi/acpica/evmisc.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/acpi/acpica/evmisc.c b/drivers/acpi/acpica/evmisc.c
index d0b3318..a78aecc 100644
--- a/drivers/acpi/acpica/evmisc.c
+++ b/drivers/acpi/acpica/evmisc.c
@@ -181,9 +181,16 @@ acpi_ev_queue_notify_request(struct acpi_namespace_node * node,
notify_info->notify.value = (u16) notify_value;
notify_info->notify.handler_obj = handler_obj;
- status =
- acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_notify_dispatch,
- notify_info);
+ /* Is this an Ejection Request? */
+ if (notify_value == 0x03)
+ status =
+ acpi_os_hotplug_execute(acpi_ev_notify_dispatch,
+ notify_info);
+ else
+ status = acpi_os_execute(OSL_NOTIFY_HANDLER,
+ acpi_ev_notify_dispatch,
+ notify_info);
+
if (ACPI_FAILURE(status)) {
acpi_ut_delete_generic_state(notify_info);
}
--
1.6.5.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests
2011-09-22 23:16 [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests Prarit Bhargava
@ 2011-09-23 1:08 ` Bjorn Helgaas
2011-09-23 1:13 ` Matthew Garrett
2011-09-23 10:48 ` Prarit Bhargava
0 siblings, 2 replies; 6+ messages in thread
From: Bjorn Helgaas @ 2011-09-23 1:08 UTC (permalink / raw)
To: Prarit Bhargava; +Cc: linux-acpi, mjg
On Thu, Sep 22, 2011 at 5:16 PM, Prarit Bhargava <prarit@redhat.com> wrote:
> This patch resolves a process hang in which a PCI card with a PCI-to-PCI
> bridge is removed via the acpiphp driver.
>
> The issue is that during the remove of the card an ACPI event occurs
> which is erroneously added the kacpi_notify_wq queue. This add is done in
> acpi_ev_queue_notify_request() via a call to acpi_os_execute().
>
> Eventually, the event runs on the kacpi_notify_wq and the code attempts to
> remove the card. During the hotplug remove of the device, the following
> call sequence happens
>
> cleanup_p2p_bridge()
> -> cleanup_bridge()
> -> acpi_remove_notify_handler()
> -> acpi_os_wait_events_complete()
> -> flush_workqueue(kacpi_notify_wq)
>
> which is the queue we are currently executing on and the process will hang.
>
> The problem is that the event is placed on the wrong queue. The code already
> contains a kacpi_hotplug_wq queue for hotplug events to be added to in order
> to prevent hangs like this.
Ugh. I see the issue, and I see how this patch works around it.
But I don't really like it because we're adding complexity and (I
think) extending knowledge of the special hotplug queue into the CA,
when I don't think we should have added the hotplug queue in the first
place.
I think the main reason we need the hotplug queue (added in
c02256be79a) is that we have driver .remove() methods calling
acpi_os_wait_events_complete() via acpi_remove_notify_handler().
I would argue that drivers should not be using
acpi_remove_notify_handler() in the first place. Adding/removing
notify handlers should be done by the ACPI core itself. The acpiphp
notify handlers (handle_hotplug_event_bridge() and
handle_hotplug_event_func()) are for bus-level, device-independent
events that should be handled not by the driver, but by the ACPI core.
Obviously, the ACPI core doesn't have hotplug support today (it has
nice "TBDs" in the EJECT_REQUEST/DEVICE_CHECK cases), so I guess all I
can do is complain without proposing a good alternative. I hate that,
sorry :)
I think this is a very important issue and merits a Bugzilla reference
so we can revisit it in the future.
> Cc: mjg@redhat.com
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> ---
> drivers/acpi/acpica/evmisc.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/acpica/evmisc.c b/drivers/acpi/acpica/evmisc.c
> index d0b3318..a78aecc 100644
> --- a/drivers/acpi/acpica/evmisc.c
> +++ b/drivers/acpi/acpica/evmisc.c
> @@ -181,9 +181,16 @@ acpi_ev_queue_notify_request(struct acpi_namespace_node * node,
> notify_info->notify.value = (u16) notify_value;
> notify_info->notify.handler_obj = handler_obj;
>
> - status =
> - acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_notify_dispatch,
> - notify_info);
> + /* Is this an Ejection Request? */
> + if (notify_value == 0x03)
Don't you want ACPI_NOTIFY_EJECT_REQUEST here instead of "0x03"?
I'm not confident that EJECT_REQUEST is the only value that might be a
problem. For example, ACPI_NOTIFY_DEVICE_CHECK in
handle_hotplug_event_{bridge,func} () leads to acpiphp_check_bridge(),
which can also wind up calling acpi_remove_notify_handler().
> + status =
> + acpi_os_hotplug_execute(acpi_ev_notify_dispatch,
> + notify_info);
> + else
> + status = acpi_os_execute(OSL_NOTIFY_HANDLER,
> + acpi_ev_notify_dispatch,
> + notify_info);
> +
> if (ACPI_FAILURE(status)) {
> acpi_ut_delete_generic_state(notify_info);
> }
> --
> 1.6.5.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests
2011-09-23 1:08 ` Bjorn Helgaas
@ 2011-09-23 1:13 ` Matthew Garrett
2011-09-23 15:29 ` Bjorn Helgaas
2011-09-23 10:48 ` Prarit Bhargava
1 sibling, 1 reply; 6+ messages in thread
From: Matthew Garrett @ 2011-09-23 1:13 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Prarit Bhargava, linux-acpi
We're seeing an increasing number of machines that use acpi
notifications to trigger rescans of devices - for instance, SD readers
that only appear on the PCI bus once a card has been inserted. These
often fail to flag themselves as removable or ejectable, so the acpiphp
core never binds. I think we need to fix this pretty urgently, for PCI
if nothing else.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests
2011-09-23 1:08 ` Bjorn Helgaas
2011-09-23 1:13 ` Matthew Garrett
@ 2011-09-23 10:48 ` Prarit Bhargava
1 sibling, 0 replies; 6+ messages in thread
From: Prarit Bhargava @ 2011-09-23 10:48 UTC (permalink / raw)
To: linux-acpi; +Cc: Bjorn Helgaas, mjg
On 09/22/2011 09:08 PM, Bjorn Helgaas wrote:
>> The problem is that the event is placed on the wrong queue. The code already
>> contains a kacpi_hotplug_wq queue for hotplug events to be added to in order
>> to prevent hangs like this.
>
> Ugh. I see the issue, and I see how this patch works around it.
>
> But I don't really like it because we're adding complexity and (I
> think) extending knowledge of the special hotplug queue into the CA,
> when I don't think we should have added the hotplug queue in the first
> place.
>
> I think the main reason we need the hotplug queue (added in
> c02256be79a) is that we have driver .remove() methods calling
> acpi_os_wait_events_complete() via acpi_remove_notify_handler().
Yup -- the documentation implies that's why it was added.
>
> I would argue that drivers should not be using
> acpi_remove_notify_handler() in the first place. Adding/removing
> notify handlers should be done by the ACPI core itself. The acpiphp
> notify handlers (handle_hotplug_event_bridge() and
> handle_hotplug_event_func()) are for bus-level, device-independent
> events that should be handled not by the driver, but by the ACPI core.
>
> Obviously, the ACPI core doesn't have hotplug support today (it has
> nice "TBDs" in the EJECT_REQUEST/DEVICE_CHECK cases), so I guess all I
> can do is complain without proposing a good alternative. I hate that,
> sorry :)
:) It's okay ... I understand what you're saying. It would be a
significant rewrite and modification of the code to get this working
"right".
>
> I think this is a very important issue and merits a Bugzilla reference
> so we can revisit it in the future.
I'll open up a BZ.
>
>> Cc: mjg@redhat.com
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> ---
>> drivers/acpi/acpica/evmisc.c | 13 ++++++++++---
>> 1 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/acpica/evmisc.c b/drivers/acpi/acpica/evmisc.c
>> index d0b3318..a78aecc 100644
>> --- a/drivers/acpi/acpica/evmisc.c
>> +++ b/drivers/acpi/acpica/evmisc.c
>> @@ -181,9 +181,16 @@ acpi_ev_queue_notify_request(struct acpi_namespace_node * node,
>> notify_info->notify.value = (u16) notify_value;
>> notify_info->notify.handler_obj = handler_obj;
>>
>> - status =
>> - acpi_os_execute(OSL_NOTIFY_HANDLER, acpi_ev_notify_dispatch,
>> - notify_info);
>> + /* Is this an Ejection Request? */
>> + if (notify_value == 0x03)
>
> Don't you want ACPI_NOTIFY_EJECT_REQUEST here instead of "0x03"?
Ah :) I knew that value had to be #define'd somewhere :) I just couldn't
find it ;). I'll fix that up.
>
> I'm not confident that EJECT_REQUEST is the only value that might be a
> problem. For example, ACPI_NOTIFY_DEVICE_CHECK in
> handle_hotplug_event_{bridge,func} () leads to acpiphp_check_bridge(),
> which can also wind up calling acpi_remove_notify_handler().
I'm only testing an EJECT_REQUEST at this point but I see your
point. I'll also add in a DEVICE_CHECK in [v2].
P.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests
2011-09-23 1:13 ` Matthew Garrett
@ 2011-09-23 15:29 ` Bjorn Helgaas
2011-09-23 15:37 ` Matthew Garrett
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2011-09-23 15:29 UTC (permalink / raw)
To: Matthew Garrett; +Cc: Prarit Bhargava, linux-acpi
On Thu, Sep 22, 2011 at 7:13 PM, Matthew Garrett <mjg@redhat.com> wrote:
> We're seeing an increasing number of machines that use acpi
> notifications to trigger rescans of devices - for instance, SD readers
> that only appear on the PCI bus once a card has been inserted. These
> often fail to flag themselves as removable or ejectable, so the acpiphp
> core never binds. I think we need to fix this pretty urgently, for PCI
> if nothing else.
Can you elaborate on this a bit? I certainly agree that Prarit's
process hang needs to be fixed, but it sounds like you're talking
about the larger issue of the ACPI core not supporting hotplug.
I think that since the core doesn't do hotplug, we can't deal with
hot-add of any ACPI devices unless we have already loaded a driver
that has hotplug support for that device. And acpiphp is not really a
"driver," in the sense that it doesn't bind to a specific PNP ID and
there isn't a good way to autoload it.
If we moved most of the notification parts of acpiphp into the core,
maybe that would be a step towards handling these SD readers and
similar devices?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests
2011-09-23 15:29 ` Bjorn Helgaas
@ 2011-09-23 15:37 ` Matthew Garrett
0 siblings, 0 replies; 6+ messages in thread
From: Matthew Garrett @ 2011-09-23 15:37 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: Prarit Bhargava, linux-acpi
On Fri, Sep 23, 2011 at 09:29:19AM -0600, Bjorn Helgaas wrote:
> On Thu, Sep 22, 2011 at 7:13 PM, Matthew Garrett <mjg@redhat.com> wrote:
> > We're seeing an increasing number of machines that use acpi
> > notifications to trigger rescans of devices - for instance, SD readers
> > that only appear on the PCI bus once a card has been inserted. These
> > often fail to flag themselves as removable or ejectable, so the acpiphp
> > core never binds. I think we need to fix this pretty urgently, for PCI
> > if nothing else.
>
> Can you elaborate on this a bit? I certainly agree that Prarit's
> process hang needs to be fixed, but it sounds like you're talking
> about the larger issue of the ACPI core not supporting hotplug.
Yup.
> I think that since the core doesn't do hotplug, we can't deal with
> hot-add of any ACPI devices unless we have already loaded a driver
> that has hotplug support for that device. And acpiphp is not really a
> "driver," in the sense that it doesn't bind to a specific PNP ID and
> there isn't a good way to autoload it.
Indeed. Worse, it only binds to devices that have _RMV or _EJ* methods.
> If we moved most of the notification parts of acpiphp into the core,
> maybe that would be a step towards handling these SD readers and
> similar devices?
I think so, yes.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-23 15:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-22 23:16 [PATCH] acpica events: Call acpi_os_hotplug_execute on Ejection Requests Prarit Bhargava
2011-09-23 1:08 ` Bjorn Helgaas
2011-09-23 1:13 ` Matthew Garrett
2011-09-23 15:29 ` Bjorn Helgaas
2011-09-23 15:37 ` Matthew Garrett
2011-09-23 10:48 ` Prarit Bhargava
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.