| Message ID | 1436701.V8egnZxOqz@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Friday, June 21, 2013 10:36:05 AM Alexander E. Patrakov wrote: > 2013/6/21 Rafael J. Wysocki <rjw@sisk.pl>: > > On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: > >> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. Well, this particular patch maybe not, but I like the direction better. Here's the relevant piece of your dmesg: [ 43.635516] ACPI: \_SB_.DOCK: undocking [ 44.108267] acpiphp_glue: _handle_hotplug_event_func: Device eject notify on \_SB_.PCI0.RP07.LPMB [ 44.110349] xhci_hcd 0000:1b:00.0: remove, state 4 [ 44.110497] usb usb6: USB disconnect, device number 1 [ 44.112203] port1: Oops, 'acpi_handle' corrupt [ 44.112242] port2: Oops, 'acpi_handle' corrupt What happens here is that USB ports are in the list of dock dependent devices, but they don't have the ops pointer set, so dock_remove_acpi_device() will be called for them *before* the usual USB teardown is triggered from the acpiphp_glue code for their parent. I *think* the solution may be to simply skip those things from ds->dependent_list (as they depend on something already in that list anyway). I'll attach an updated patch with that change to the bug entry. [ 44.112332] xHCI xhci_drop_endpoint called for root hub [ 44.112334] xHCI xhci_check_bandwidth called for root hub [ 44.114313] usb usb6: Oops, 'acpi_handle' corrupt Same thing again. [ 44.114349] xhci_hcd 0000:1b:00.0: Host not halted after 16000 microseconds. [ 44.114351] xhci_hcd 0000:1b:00.0: USB bus 6 deregistered [ 44.114360] xhci_hcd 0000:1b:00.0: remove, state 4 [ 44.114392] usb usb5: USB disconnect, device number 1 [ 44.114394] usb 5-1: USB disconnect, device number 2 [ 49.120740] xhci_hcd 0000:1b:00.0: Timeout while waiting for configure endpoint command [ 49.120750] xhci_hcd 0000:1b:00.0: Stopped the command ring failed, maybe the host is dead [ 49.120757] xhci_hcd 0000:1b:00.0: Host not halted after 16000 microseconds. [ 49.120759] xhci_hcd 0000:1b:00.0: Abort command ring failed [ 49.120760] xhci_hcd 0000:1b:00.0: HC died; cleaning up [ 49.122075] xHCI xhci_drop_endpoint called for root hub [ 49.122076] xHCI xhci_check_bandwidth called for root hub [ 49.123825] usb usb5: Oops, 'acpi_handle' corrupt Same thing again. [ 49.123906] xhci_hcd 0000:1b:00.0: Host not halted after 16000 microseconds. [ 49.124208] xhci_hcd 0000:1b:00.0: USB bus 5 deregistered [ 52.150756] ata8.00: disabled [ 52.172219] r8169 0000:19:00.0 enp25s0: rtl_counters_cond == 1 (loop: 1000, delay: 10). [ 52.182516] r8169 0000:19:00.0 enp25s0: rtl_chipcmd_cond == 1 (loop: 100, delay: 100). [ 52.188939] r8169 0000:19:00.0 enp25s0: rtl_phyar_cond == 1 (loop: 20, delay: 25). [ 52.189542] r8169 0000:19:00.0 enp25s0: rtl_phyar_cond == 1 (loop: 20, delay: 25). [ 52.190191] r8169 0000:19:00.0 enp25s0: rtl_phyar_cond == 1 (loop: 20, delay: 25). [ 52.190746] r8169 0000:19:00.0 enp25s0: rtl_phyar_cond == 1 (loop: 20, delay: 25). [ 52.191299] r8169 0000:19:00.0 enp25s0: rtl_phyar_cond == 1 (loop: 20, delay: 25). [ 52.212009] r8169 0000:19:00.0 (unregistered net_device): rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 52.222185] r8169 0000:19:00.0 (unregistered net_device): rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 52.232357] r8169 0000:19:00.0 (unregistered net_device): rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 52.242529] r8169 0000:19:00.0 (unregistered net_device): rtl_eriar_cond == 1 (loop: 100, delay: 100). [ 52.260158] vgaarb: device changed decodes: PCI:0000:00:02.0,olddecodes=none,decodes=io+mem:owns=none [ 52.265799] [drm] radeon: finishing device. [ 52.265804] [drm] Disabling audio support [ 52.267668] radeon 0000:16:00.0: ffff880251e28400 unpin not necessary [ 52.268033] [drm:drm_mm_takedown] *ERROR* Memory manager not clean. Delaying takedown [ 52.268164] [TTM] Finalizing pool allocator [ 52.268340] [TTM] Finalizing DMA pool allocator The thing below looks like a radeon problem? [ 52.268389] ------------[ cut here ]------------ [ 52.268395] WARNING: at drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:533 ttm_dma_free_pool+0x101/0x110 [ttm]() [ 52.268396] Modules linked in: ata_generic pata_acpi pata_marvell radeon ttm zram(C) rfcomm bnep rtsx_pci_ms rtsx_pci_sdmmc mmc_core memstick iTCO_wdt iTCO_vendor_support coretemp kvm_intel kvm joydev pcspkr i2c_i801 uvcvideo qcserial usb_wwan videobuf2_vmalloc videobuf2_memops videobuf2_core usbserial btusb videodev bluetooth arc4 media iwldvm mac80211 snd_hda_codec_hdmi iwlwifi r8169 rtsx_pci mii cfg80211 snd_hda_codec_realtek i915 sony_laptop rfkill intel_agp snd_hda_intel intel_gtt snd_hda_codec i2c_algo_bit snd_hwdep drm_kms_helper snd_pcm snd_page_alloc drm snd_timer agpgart lpc_ich snd mfd_core sha256_ssse3 sha256_generic dm_crypt raid0 md_mod crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd xhci_hcd ehci_pci ehci_hcd dm_mirror [ 52.268444] dm_region_hash dm_log dm_mod [last unloaded: microcode] [ 52.268449] CPU: 0 PID: 4 Comm: kworker/0:0 Tainted: G C 3.10.0-rc6-rafael #1 [ 52.268450] Hardware name: Sony Corporation VPCZ23A4R/VAIO, BIOS R1013H5 05/21/2012 [ 52.268455] Workqueue: kacpi_hotplug _handle_hotplug_event_func [ 52.268456] ffffffffa081f268 ffff8802540d38b8 ffffffff8165af98 ffff8802540d38f8 [ 52.268460] ffffffff8103c8cb ffff8802540d38c8 ffff880251f53700 ffff8802513e23c0 [ 52.268463] 0000000000000008 ffff880253d02530 ffff880254079730 ffff8802540d3908 [ 52.268466] Call Trace: [ 52.268470] [<ffffffff8165af98>] dump_stack+0x19/0x1b [ 52.268474] [<ffffffff8103c8cb>] warn_slowpath_common+0x6b/0xa0 [ 52.268476] [<ffffffff8103c915>] warn_slowpath_null+0x15/0x20 [ 52.268480] [<ffffffffa081cd01>] ttm_dma_free_pool+0x101/0x110 [ttm] [ 52.268484] [<ffffffffa081dee5>] ttm_dma_page_alloc_fini+0x85/0xcc [ttm] [ 52.268487] [<ffffffffa0813455>] ttm_mem_global_release+0x25/0xa0 [ttm] [ 52.268497] [<ffffffffa08494ed>] radeon_ttm_mem_global_release+0xd/0x10 [radeon] [ 52.268507] [<ffffffffa0176213>] drm_global_item_unref+0x63/0x90 [drm] [ 52.268514] [<ffffffffa084a721>] radeon_ttm_fini+0xd1/0xe0 [radeon] [ 52.268522] [<ffffffffa084b129>] radeon_bo_fini+0x9/0x10 [radeon] [ 52.268531] [<ffffffffa0893bf1>] evergreen_fini+0x91/0xc0 [radeon] [ 52.268537] [<ffffffffa08336ea>] radeon_device_fini+0x3a/0xf0 [radeon] [ 52.268543] [<ffffffffa08353d1>] radeon_driver_unload_kms+0x41/0x70 [radeon] [ 52.268550] [<ffffffffa016524e>] drm_put_dev+0x6e/0x210 [drm] [ 52.268555] [<ffffffffa0832068>] radeon_pci_remove+0x18/0x20 [radeon] [ 52.268557] [<ffffffff81385831>] pci_device_remove+0x41/0xc0 [ 52.268561] [<ffffffff8143bfd7>] __device_release_driver+0x77/0xe0 [ 52.268563] [<ffffffff8143c069>] device_release_driver+0x29/0x40 [ 52.268566] [<ffffffff8143ba71>] bus_remove_device+0xf1/0x140 [ 52.268568] [<ffffffff8143915d>] device_del+0x11d/0x1b0 [ 52.268572] [<ffffffff8138082c>] pci_stop_bus_device+0x9c/0xb0 [ 52.268574] [<ffffffff813807cb>] pci_stop_bus_device+0x3b/0xb0 [ 52.268576] [<ffffffff813a11e5>] ? acpiphp_disable_slot+0x35/0x140 [ 52.268579] [<ffffffff813807cb>] pci_stop_bus_device+0x3b/0xb0 [ 52.268581] [<ffffffff813807cb>] pci_stop_bus_device+0x3b/0xb0 [ 52.268584] [<ffffffff813807cb>] pci_stop_bus_device+0x3b/0xb0 [ 52.268586] [<ffffffff81380991>] pci_stop_and_remove_bus_device+0x11/0x20 [ 52.268588] [<ffffffff813a1236>] acpiphp_disable_slot+0x86/0x140 [ 52.268591] [<ffffffff813a1622>] _handle_hotplug_event_func+0x102/0x1e0 [ 52.268594] [<ffffffff8105c2c2>] process_one_work+0x1c2/0x560 [ 52.268597] [<ffffffff8105c257>] ? process_one_work+0x157/0x560 [ 52.268599] [<ffffffff8105d1d6>] worker_thread+0x116/0x370 [ 52.268601] [<ffffffff8105d0c0>] ? manage_workers.isra.20+0x2d0/0x2d0 [ 52.268604] [<ffffffff81063a36>] kthread+0xd6/0xe0 [ 52.268606] [<ffffffff816611ab>] ? _raw_spin_unlock_irq+0x2b/0x60 [ 52.268609] [<ffffffff81063960>] ? __init_kthread_worker+0x70/0x70 [ 52.268612] [<ffffffff8166856c>] ret_from_fork+0x7c/0xb0 [ 52.268615] [<ffffffff81063960>] ? __init_kthread_worker+0x70/0x70 [ 52.268669] ---[ end trace 46b4e977738a3df6 ]--- [ 52.269258] [TTM] Zone kernel: Used memory at exit: 13 kiB [ 52.269266] [TTM] Zone dma32: Used memory at exit: 9 kiB [ 52.269295] [drm] radeon: ttm finalized But it looks like the removal actually succeeded. [ 52.273103] pci_bus 0000:0b: busn_res: [bus 0b] is released [ 52.273732] pci_bus 0000:0c: busn_res: [bus 0c] is released [ 52.273816] pci_bus 0000:16: busn_res: [bus 16] is released Is the re-dock attempt included? It doesn't seem to leave any trace ... Thanks, Rafael
2013/6/21 Rafael J. Wysocki <rjw@sisk.pl>: > The thing below looks like a radeon problem? > > [ 52.268389] ------------[ cut here ]------------ > [ 52.268395] WARNING: at drivers/gpu/drm/ttm/ttm_page_alloc_dma.c:533 ttm_dma_free_pool+0x101/0x110 [ttm]() Yes. Let's ignore it for now, as it is a separate bug, likely a special case of https://bugzilla.kernel.org/show_bug.cgi?id=59681 > But it looks like the removal actually succeeded. It did. > [ 52.273103] pci_bus 0000:0b: busn_res: [bus 0b] is released > [ 52.273732] pci_bus 0000:0c: busn_res: [bus 0c] is released > [ 52.273816] pci_bus 0000:16: busn_res: [bus 16] is released > > Is the re-dock attempt included? It doesn't seem to leave any trace ... It is, and indeed it left no traces. See the followup when I attached the result of manually rescanning the bus - you have already commented on the deadlock there.
On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: > On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: >> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. > > 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. Hi Rafael, There's an ongoing patch to fix a disk bay hotplug regression, which may add a second caller of register_hotplug_device(). Please refer to bug 59871, and the proposed patch is at: https://bugzilla.kernel.org/attachment.cgi?id=105581 > > 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); > + } The proposed patch for bug 59871 may not be safe with above changes because the ACPI ATA hotplug handler may not remove ACPI devices as acpiphp driver does. On the other hand, the above change does get rid of the warning message "Oops, 'acpi_handle' corrupt", but it may hide the real issue. With current implementation, devices on the dock station are stopped and removed after invoking ACPI _DCK method, which seems a little dangerous. I think ACPI _DCK method should be called to power off the dock station after stopping all affected devices. Regards! Gerry > + > + 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); > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, June 22, 2013 12:54:21 AM Jiang Liu wrote: > On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: > > On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: > >> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. > > > > 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. > Hi Rafael, > There's an ongoing patch to fix a disk bay hotplug regression, which > may add a second caller of register_hotplug_device(). Please refer to > bug 59871, and the proposed patch is at: > https://bugzilla.kernel.org/attachment.cgi?id=105581 > > > > > 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); > > + } > The proposed patch for bug 59871 may not be safe with above changes > because the ACPI ATA hotplug handler may not remove ACPI devices as > acpiphp driver does. Well, since there's not ATA hotplug handler in 3.10-rc6, as far as I can say, I suppose that you're talking about a new patch scheduled for 3.11. If so, can you please give me a pointer to that patch, possibly the tree it is queued on etc.? > On the other hand, the above change does get rid of the warning message > "Oops, 'acpi_handle' corrupt", but it may hide the real issue. With > current implementation, devices on the dock station are stopped and > removed after invoking ACPI _DCK method, which seems a little dangerous. Yes, that's something I actually overlooked. In fact, the execution of _DCK has to wait for all of the asynchronous work items spawned by hotplug_dock_devices() to complete, so we *have* *to* run the acpiphp stuff (the internals of _handle_hotplug_event_func() basically) synchronously from the dock context. > I think ACPI _DCK method should be called to power off the dock station > after stopping all affected devices. That's correct. Not really to power off, but to disconnect ("isolate from connector" as the spec puts that), although from the kernel's point of view the result is pretty much the same - the devices are gone. I have attached a new patch for Alexander to try at https://bugzilla.kernel.org/show_bug.cgi?id=59501#c25 It kind of combines your patches [2/4] and [3/4] from the $subject series with my last patch. The most obvious difference is that it doesn't use klists. :-) Seriously, I really think we don't need a separate "hotplug devices" list for docking stations, one "dependent devices" list should be sufficient for everything (it doesn't change anyway after the initialization), but I stole your idea with the "get" and "put" routines (I called them "init" and "release"). However, I didn't add them to struct acpi_dock_ops, but modified register_hotplug_dock_device() to pass them directly instead (I believe this is less prone to errors that way, because the callers of register_hotplug_dock_device() cannot really overlook them). We still may need to modify find_dock_devices() on top of that. To summarize, we have multiple different problems in that code. First, there is the ordering issue of the dock initialization versus PCI enumeration, so basically dock_init() has to run before the main ACPI namespace scan. This is addressed by your patch [1/4] in the $subject series, but I'm not 100% happy with that approach (I believe we need it as a stopgap fix for now, though), because it means we have to carry out a full namespace walk (possibly several of them even) before we even start to create struct acpi_device objects and that doesn't sound quite right. Second, there is the resources allocation issue addressed by your patch [4/4] from the $subject series. I believe that this patch is correct and Yinghai seems to agree with me. Next, there is the problem with asynchronous handling of dock events by acpiphp that we're trying to solve at the moment and a couple of things are clear to me here: 1. Clearly, the dock driver assumes that dd->ops->handler() will always be synchronous, because otherwise there would have been a completion mechanism preventing _DCK from being executed before all of the handlers return. Since there's no such thing, we're dealing with a genuine acpiphp bug in its implementation of the dock handler and I'm not sure how it's ever been supposed to work. 2. Whoever wrote find_dock_devices() didn't seem to know how acpi_bus_scan() worked, or that function would have been arranged differently and (moreover) the whay acpi_bus_scan() works has changed since then, so the assumptions made there may not be valid any more. Moreover, it looks like ds->dependent_list should be walked in the reverse order during removal. 3. When we remove dock_{add|del}_hotplug_device(), this way or another, ds->hp_lock will be pointless, because its only user is hotplug_dock_devices() and it is always called under acpi_scan_lock. Finally, there seem to be problems with PCI device drivers' .remove() callbacks not working correctly in some cases and causing problems to happen. Thanks, Rafael
On 06/22/2013 08:13 AM, Rafael J. Wysocki wrote: > On Saturday, June 22, 2013 12:54:21 AM Jiang Liu wrote: >> On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: >>> On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: >>>> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. >>> >>> 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. >> Hi Rafael, >> There's an ongoing patch to fix a disk bay hotplug regression, which >> may add a second caller of register_hotplug_device(). Please refer to >> bug 59871, and the proposed patch is at: >> https://bugzilla.kernel.org/attachment.cgi?id=105581 Hi Rafael, You can find the proposed patch for ATA Bay hotplug at https://bugzilla.kernel.org/attachment.cgi?id=105581 Regards! Gerry >> >> The proposed patch for bug 59871 may not be safe with above changes >> because the ACPI ATA hotplug handler may not remove ACPI devices as >> acpiphp driver does. > > Well, since there's not ATA hotplug handler in 3.10-rc6, as far as I can say, > I suppose that you're talking about a new patch scheduled for 3.11. If so,u > can you please give me a pointer to that patch, possibly the tree it is > queued on etc.? [...] > > Thanks, > Rafael > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday, June 22, 2013 10:47:24 AM Jiang Liu wrote: > On 06/22/2013 08:13 AM, Rafael J. Wysocki wrote: > > On Saturday, June 22, 2013 12:54:21 AM Jiang Liu wrote: > >> On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: > >>> On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: > >>>> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. > >>> > >>> 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. > >> Hi Rafael, > >> There's an ongoing patch to fix a disk bay hotplug regression, which > >> may add a second caller of register_hotplug_device(). Please refer to > >> bug 59871, and the proposed patch is at: > >> https://bugzilla.kernel.org/attachment.cgi?id=105581 > Hi Rafael, > You can find the proposed patch for ATA Bay hotplug at > https://bugzilla.kernel.org/attachment.cgi?id=105581 Well, maybe it'll have to be reworked slightly. Which BZ entry is that? Rafael
On 06/23/2013 03:59 AM, Rafael J. Wysocki wrote: > On Saturday, June 22, 2013 10:47:24 AM Jiang Liu wrote: >> On 06/22/2013 08:13 AM, Rafael J. Wysocki wrote: >>> On Saturday, June 22, 2013 12:54:21 AM Jiang Liu wrote: >>>> On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: >>>>> On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: >>>>>> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. >>>>> >>>>> 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. >>>> Hi Rafael, >>>> There's an ongoing patch to fix a disk bay hotplug regression, which >>>> may add a second caller of register_hotplug_device(). Please refer to >>>> bug 59871, and the proposed patch is at: >>>> https://bugzilla.kernel.org/attachment.cgi?id=105581 >> Hi Rafael, >> You can find the proposed patch for ATA Bay hotplug at >> https://bugzilla.kernel.org/attachment.cgi?id=105581 > > Well, maybe it'll have to be reworked slightly. > > Which BZ entry is that? Hi Rafael, It's bug 59871, please refer to: https://bugzilla.kernel.org/show_bug.cgi?id=59871 Regards! Gerry > > Rafael > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, June 23, 2013 11:57:46 PM Jiang Liu wrote: > On 06/23/2013 03:59 AM, Rafael J. Wysocki wrote: > > On Saturday, June 22, 2013 10:47:24 AM Jiang Liu wrote: > >> On 06/22/2013 08:13 AM, Rafael J. Wysocki wrote: > >>> On Saturday, June 22, 2013 12:54:21 AM Jiang Liu wrote: > >>>> On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: > >>>>> On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: > >>>>>> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. > >>>>> > >>>>> 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. > >>>> Hi Rafael, > >>>> There's an ongoing patch to fix a disk bay hotplug regression, which > >>>> may add a second caller of register_hotplug_device(). Please refer to > >>>> bug 59871, and the proposed patch is at: > >>>> https://bugzilla.kernel.org/attachment.cgi?id=105581 > >> Hi Rafael, > >> You can find the proposed patch for ATA Bay hotplug at > >> https://bugzilla.kernel.org/attachment.cgi?id=105581 > > > > Well, maybe it'll have to be reworked slightly. > > > > Which BZ entry is that? > Hi Rafael, > It's bug 59871, please refer to: > https://bugzilla.kernel.org/show_bug.cgi?id=59871 OK, thanks! I suppose it would be most convenient to simply add that patch to this patchset. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday, June 23, 2013 11:57:46 PM Jiang Liu wrote: > On 06/23/2013 03:59 AM, Rafael J. Wysocki wrote: > > On Saturday, June 22, 2013 10:47:24 AM Jiang Liu wrote: > >> On 06/22/2013 08:13 AM, Rafael J. Wysocki wrote: > >>> On Saturday, June 22, 2013 12:54:21 AM Jiang Liu wrote: > >>>> On 06/21/2013 03:06 AM, Rafael J. Wysocki wrote: > >>>>> On Wednesday, June 19, 2013 11:18:41 AM Alexander E. Patrakov wrote: > >>>>>> 2013/6/19 Rafael J. Wysocki <rjw@sisk.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. > >>>>> > >>>>> 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. > >>>> Hi Rafael, > >>>> There's an ongoing patch to fix a disk bay hotplug regression, which > >>>> may add a second caller of register_hotplug_device(). Please refer to > >>>> bug 59871, and the proposed patch is at: > >>>> https://bugzilla.kernel.org/attachment.cgi?id=105581 > >> Hi Rafael, > >> You can find the proposed patch for ATA Bay hotplug at > >> https://bugzilla.kernel.org/attachment.cgi?id=105581 > > > > Well, maybe it'll have to be reworked slightly. > > > > Which BZ entry is that? > Hi Rafael, > It's bug 59871, please refer to: > https://bugzilla.kernel.org/show_bug.cgi?id=59871 OK, thanks! I suppose it would be most convenient to simply add that patch to this patchset. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);