| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAN_LGv2kZy_8SrKae5YUwHoUM+mf1OgH2HkAEtiC+ojKCFYSeg@mail.gmail.com>
Date: Fri, 21 Jun 2013 10:36:05 +0600
From: "Alexander E. Patrakov" <patrakov@...il.com>
To: "Rafael J. Wysocki" <rjw@...k.pl>
Cc: Jiang Liu <jiang.liu@...wei.com>, alexdeucher@...il.com,
Bjorn Helgaas <bhelgaas@...gle.com>,
Yinghai Lu <yinghai@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Yijing Wang <wangyijing@...wei.com>,
linux-acpi@...r.kernel.org, Jiang Liu <liuj97@...il.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [BUGFIX v2 0/4] fix bug 56531, 59501 and 59581
2013/6/21 Rafael J. Wysocki <rjw@...k.pl>:
> On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote:
>> 2013/6/19 Rafael J. Wysocki <rjw@...k.pl>:
>> > OK, let's try to untangle this a bit.
>> >
>> > If you applyt patches [1/4] and [4/4] from the $subject series only, what
>> > does remain unfixed?
>>
>> [not tested, can do so in 12 hours if needed]
>>
>> I think there will be problems on undocking and/or on the second
>> docking, as described in comments #6 - #8 of
>> https://bugzilla.kernel.org/show_bug.cgi?id=59501
>
> OK, I think I have something that might work. It may not solve all problems,
> but maybe it helps a bit. Unfortunately, I can't really test it, so please do
> if you can.
>
> Please apply [1/4] and [4/4] and the one below and see what happens.
Tested on top of 3.10-rc6.
Attached dmesg output for the following testcase: boot undocked, dock,
undock, dock.
The initial dock went OK. The subsequent undock resulted in the blue
led on the dock cable turning off quickly, but in PCI devices slowly,
one-by-one, disappearing from the bus. Also, there were "acpi_handle
corrupt" messages in dmesg. The subsequent dock resulted in no devices
added to the bus. So - your patch is not a good replacement for
patches 2 and 3 in the original series.
>
> Thanks,
> Rafael
>
>
> ---
> Rationale:
> acpiphp_glue.c:disable_device() trims the underlying ACPI device objects
> after removing the companion PCI devices, so the dock station code
> doesn't need to trim them separately for the dependent devices handled
> by acpiphp.
>
> Moreover, acpiphp_glue.c is the only user of
> [un]register_hotplug_dock_device(), so *all* devices on the
> ds->hotplug_devices list are handled by acpiphp and ops is set for all
> of them.
>
> This means that (1) the ds->hotplug_devices list is not necessary (we
> can always walk ds->dependent_devices instead and look for those that
> have dd->ops set) and (2) we don't need to call
> dock_remove_acpi_device(dd->handle) on eject for any of those devices,
> because dd->ops->handler() is going to take care of the ACPI device
> objects trimming for them anyway.
>
> Taking the above into account make the following changes:
> (1) Drop hotplug_devices from struct dock_station.
> (2) Drop dock_{add|del}_hotplug_device()
> (3) Make [un]register_hotplug_dock_device() [un]set 'ops' and
> 'context' for the given device under ds->hp_lock.
> (4) Add hot_remove_dock_devices() that walks ds->dependent_devices and
> either calls dd->ops->handler(), if present, or trims the underlying
> ACPI device object, otherwise.
> (5) Replace hotplug_dock_devices(ds, ACPI_NOTIFY_EJECT_REQUEST) calls
> with hot_remove_dock_devices(ds).
> (6) Rename hotplug_dock_devices() to hot_add_dock_devices() and make
> it only handle bus check and device check requests. Make it walk
> ds->dependent_devices instead of ds->hotplug devices.
> (7) Make dock_event() walk ds->dependent_devices (instead of
> ds->hotplug devices) under ds->hp_lock.
> ---
> drivers/acpi/dock.c | 111 ++++++++++++++++++++++++----------------------------
> 1 file changed, 53 insertions(+), 58 deletions(-)
>
> Index: linux-pm/drivers/acpi/dock.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/dock.c
> +++ linux-pm/drivers/acpi/dock.c
> @@ -66,7 +66,6 @@ struct dock_station {
> spinlock_t dd_lock;
> struct mutex hp_lock;
> struct list_head dependent_devices;
> - struct list_head hotplug_devices;
>
> struct list_head sibling;
> struct platform_device *dock_device;
> @@ -121,38 +120,6 @@ add_dock_dependent_device(struct dock_st
> }
>
> /**
> - * dock_add_hotplug_device - associate a hotplug handler with the dock station
> - * @ds: The dock station
> - * @dd: The dependent device struct
> - *
> - * Add the dependent device to the dock's hotplug device list
> - */
> -static void
> -dock_add_hotplug_device(struct dock_station *ds,
> - struct dock_dependent_device *dd)
> -{
> - mutex_lock(&ds->hp_lock);
> - list_add_tail(&dd->hotplug_list, &ds->hotplug_devices);
> - mutex_unlock(&ds->hp_lock);
> -}
> -
> -/**
> - * dock_del_hotplug_device - remove a hotplug handler from the dock station
> - * @ds: The dock station
> - * @dd: the dependent device struct
> - *
> - * Delete the dependent device from the dock's hotplug device list
> - */
> -static void
> -dock_del_hotplug_device(struct dock_station *ds,
> - struct dock_dependent_device *dd)
> -{
> - mutex_lock(&ds->hp_lock);
> - list_del(&dd->hotplug_list);
> - mutex_unlock(&ds->hp_lock);
> -}
> -
> -/**
> * find_dock_dependent_device - get a device dependent on this dock
> * @ds: the dock station
> * @handle: the acpi_handle of the device we want
> @@ -342,40 +309,60 @@ static void dock_remove_acpi_device(acpi
> }
>
> /**
> - * hotplug_dock_devices - insert or remove devices on the dock station
> - * @ds: the dock station
> - * @event: either bus check or eject request
> + * hot_remove_dock_devices - Remove devices on a dock station.
> + * @ds: Dock station to remove devices for.
> + *
> + * For each device depending on @ds, if a dock event handler is registered,
> + * call it for the device, or trim the underlying ACPI device object otherwise.
> + *
> + * Dock event handlers are responsible for trimming the underlying ACPI device
> + * objects if present.
> + */
> +static void hot_remove_dock_devices(struct dock_station *ds)
> +{
> + struct dock_dependent_device *dd;
> +
> + mutex_lock(&ds->hp_lock);
> +
> + list_for_each_entry(dd, &ds->dependent_devices, list) {
> + if (dd->ops && dd->ops->handler)
> + dd->ops->handler(dd->handle, ACPI_NOTIFY_EJECT_REQUEST,
> + dd->context);
> + else
> + dock_remove_acpi_device(dd->handle);
> + }
> +
> + mutex_unlock(&ds->hp_lock);
> +}
> +
> +/**
> + * hot_add_dock_devices - Insert devices on a dock station.
> + * @ds: Dock station to insert devices for.
> *
> * Some devices on the dock station need to have drivers called
> * to perform hotplug operations after a dock event has occurred.
> * Traverse the list of dock devices that have registered a
> * hotplug handler, and call the handler.
> */
> -static void hotplug_dock_devices(struct dock_station *ds, u32 event)
> +static void hot_add_dock_devices(struct dock_station *ds, u32 event)
> {
> struct dock_dependent_device *dd;
>
> mutex_lock(&ds->hp_lock);
>
> - /*
> - * First call driver specific hotplug functions
> - */
> - list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
> + /* First call driver specific hotplug event handlers. */
> + list_for_each_entry(dd, &ds->dependent_devices, list)
> if (dd->ops && dd->ops->handler)
> dd->ops->handler(dd->handle, event, dd->context);
>
> /*
> - * Now make sure that an acpi_device is created for each
> - * dependent device, or removed if this is an eject request.
> - * This will cause acpi_drivers to be stopped/started if they
> - * exist
> + * Now make sure that an acpi_device is created for each dependent
> + * device. That will cause scan handlers to attach to devices objects
> + * or acpi_drivers to be started if they exist.
> */
> - list_for_each_entry(dd, &ds->dependent_devices, list) {
> - if (event == ACPI_NOTIFY_EJECT_REQUEST)
> - dock_remove_acpi_device(dd->handle);
> - else
> - dock_create_acpi_device(dd->handle);
> - }
> + list_for_each_entry(dd, &ds->dependent_devices, list)
> + dock_create_acpi_device(dd->handle);
> +
> mutex_unlock(&ds->hp_lock);
> }
>
> @@ -398,10 +385,14 @@ static void dock_event(struct dock_stati
> if (num == DOCK_EVENT)
> kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>
> - list_for_each_entry(dd, &ds->hotplug_devices, hotplug_list)
> + mutex_lock(&ds->hp_lock);
> +
> + list_for_each_entry(dd, &ds->dependent_devices, list)
> if (dd->ops && dd->ops->uevent)
> dd->ops->uevent(dd->handle, event, dd->context);
>
> + mutex_unlock(&ds->hp_lock);
> +
> if (num != DOCK_EVENT)
> kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> }
> @@ -598,9 +589,10 @@ register_hotplug_dock_device(acpi_handle
> */
> dd = find_dock_dependent_device(dock_station, handle);
> if (dd) {
> + mutex_lock(&dock_station->hp_lock);
> dd->ops = ops;
> dd->context = context;
> - dock_add_hotplug_device(dock_station, dd);
> + mutex_unlock(&dock_station->hp_lock);
> ret = 0;
> }
> }
> @@ -623,8 +615,12 @@ void unregister_hotplug_dock_device(acpi
>
> list_for_each_entry(dock_station, &dock_stations, sibling) {
> dd = find_dock_dependent_device(dock_station, handle);
> - if (dd)
> - dock_del_hotplug_device(dock_station, dd);
> + if (dd) {
> + mutex_lock(&dock_station->hp_lock);
> + dd->ops = NULL;
> + dd->context = NULL;
> + mutex_unlock(&dock_station->hp_lock);
> + }
> }
> }
> EXPORT_SYMBOL_GPL(unregister_hotplug_dock_device);
> @@ -649,7 +645,7 @@ static int handle_eject_request(struct d
> */
> dock_event(ds, event, UNDOCK_EVENT);
>
> - hotplug_dock_devices(ds, ACPI_NOTIFY_EJECT_REQUEST);
> + hot_remove_dock_devices(ds);
> undock(ds);
> dock_lock(ds, 0);
> eject_dock(ds);
> @@ -708,7 +704,7 @@ static void dock_notify(acpi_handle hand
> }
> atomic_notifier_call_chain(&dock_notifier_list,
> event, NULL);
> - hotplug_dock_devices(ds, event);
> + hot_add_dock_devices(ds, event);
> complete_dock(ds);
> dock_event(ds, event, DOCK_EVENT);
> dock_lock(ds, 1);
> @@ -953,7 +949,6 @@ static int __init dock_add(acpi_handle h
> mutex_init(&dock_station->hp_lock);
> spin_lock_init(&dock_station->dd_lock);
> INIT_LIST_HEAD(&dock_station->sibling);
> - INIT_LIST_HEAD(&dock_station->hotplug_devices);
> ATOMIC_INIT_NOTIFIER_HEAD(&dock_notifier_list);
> INIT_LIST_HEAD(&dock_station->dependent_devices);
>
>
>
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
--
Alexander E. Patrakov
View attachment "dmesg.txt" of type "text/plain" (95906 bytes)
Powered by blists - more mailing lists