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@xxxxxxxxxx
>> Signed-off-by: Prarit Bhargava <prarit@xxxxxxxxxx>
>> ---
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
