From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756295Ab3KWWxX (ORCPT ); Sat, 23 Nov 2013 17:53:23 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:35657 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755688Ab3KWWxU (ORCPT ); Sat, 23 Nov 2013 17:53:20 -0500 Date: Sat, 23 Nov 2013 14:53:58 -0800 From: Greg Kroah-Hartman To: "Rafael J. Wysocki" Cc: Bjorn Helgaas , Tejun Heo , Mika Westerberg , "linux-kernel@vger.kernel.org" , "James E.J. Bottomley" Subject: Re: [PATCH] sysfs: handle duplicate removal attempts in sysfs_remove_group() Message-ID: <20131123225358.GB5402@kroah.com> References: <1384866598-19716-1-git-send-email-mika.westerberg@linux.intel.com> <20131120061830.GA16081@mtj.dyndns.org> <346316422.RJFnQfZPnq@vostro.rjw.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <346316422.RJFnQfZPnq@vostro.rjw.lan> User-Agent: Mutt/1.5.22 (2013-10-16) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Nov 23, 2013 at 11:56:48PM +0100, Rafael J. Wysocki wrote: > On Friday, November 22, 2013 08:43:55 AM Bjorn Helgaas wrote: > > [+cc Rafael, James] > > > > On Tue, Nov 19, 2013 at 11:18 PM, Tejun Heo wrote: > > > (cc'ing Bjorn) > > > > > > Hello, > > > > > > On Tue, Nov 19, 2013 at 03:09:58PM +0200, Mika Westerberg wrote: > > >> Commit bcdde7e221a8 (sysfs: make __sysfs_remove_dir() recursive) changed > > >> the behavior so that directory removals will be done recursively. This > > >> means that the sysfs group might already be removed if its parent directory > > >> has been removed. > > >> > > >> The current code outputs warnings similar to following log snippet when it > > >> detects that there is no group for the given kobject: > > >> > > >> WARNING: CPU: 0 PID: 4 at fs/sysfs/group.c:214 sysfs_remove_group+0xc6/0xd0() > > >> sysfs group ffffffff81c6f1e0 not found for kobject 'host7' > > >> Modules linked in: > > >> CPU: 0 PID: 4 Comm: kworker/0:0 Not tainted 3.12.0+ #13 > > >> Hardware name: /D33217CK, BIOS GKPPT10H.86A.0042.2013.0422.1439 04/22/2013 > > >> Workqueue: kacpi_hotplug acpi_hotplug_work_fn > > >> 0000000000000009 ffff8801002459b0 ffffffff817daab1 ffff8801002459f8 > > >> ffff8801002459e8 ffffffff810436b8 0000000000000000 ffffffff81c6f1e0 > > >> ffff88006d440358 ffff88006d440188 ffff88006e8b4c28 ffff880100245a48 > > >> Call Trace: > > >> [] dump_stack+0x45/0x56 > > >> [] warn_slowpath_common+0x78/0xa0 > > >> [] warn_slowpath_fmt+0x47/0x50 > > >> [] sysfs_remove_group+0xc6/0xd0 > > >> [] dpm_sysfs_remove+0x3e/0x50 > > >> [] device_del+0x40/0x1b0 > > >> [] device_unregister+0xd/0x20 > > >> [] scsi_remove_host+0xba/0x110 > > >> [] ata_host_detach+0xc6/0x100 > > >> [] ata_pci_remove_one+0x18/0x20 > > >> [] pci_device_remove+0x28/0x60 > > >> [] __device_release_driver+0x64/0xd0 > > >> [] device_release_driver+0x1e/0x30 > > >> [] bus_remove_device+0xf7/0x140 > > >> [] device_del+0x121/0x1b0 > > >> [] pci_stop_bus_device+0x94/0xa0 > > >> [] pci_stop_bus_device+0x3b/0xa0 > > >> [] pci_stop_bus_device+0x3b/0xa0 > > >> [] pci_stop_and_remove_bus_device+0xd/0x20 > > >> [] trim_stale_devices+0x73/0xe0 > > >> [] trim_stale_devices+0xbb/0xe0 > > >> [] trim_stale_devices+0xbb/0xe0 > > >> [] acpiphp_check_bridge+0x7e/0xd0 > > >> [] hotplug_event+0xcd/0x160 > > >> [] hotplug_event_work+0x25/0x60 > > >> [] acpi_hotplug_work_fn+0x17/0x22 > > >> [] process_one_work+0x17a/0x430 > > >> [] worker_thread+0x119/0x390 > > >> [] kthread+0xcd/0xf0 > > >> [] ret_from_fork+0x7c/0xb0 > > > > > > So, we do have cases where the parent is removed before the child. I > > > suppose the parent pci bridge is removed already? AFAICS this > > > shouldn't break anything but people did seem to expect the removals to > > > be ordered from child to parent. Bjorn, is this something you expect > > > to happened? > > > > I do not expect a PCI bridge to be removed before the devices below > > it. We should be removing all the children before removing the parent > > bridge. > > > > But is this related to PCI? I don't see the connection yet. I tried > > to look into this a bit (my notes are at > > https://bugzilla.kernel.org/show_bug.cgi?id=65281), but I haven't > > figured out the big-picture problem yet. > > OK, so I think I've figured this out. > > As I wrote in the above BZ entry, after the Tejun's patch the > device_del() in pci_stop_dev() removes the subordinate bus object's > "power" directory among other things and because of that the warning > triggers when we do the pci_remove_bus() in pci_remove_bus_device() > for the same device. > > The appended patch fixes it for me, but Greg has already taken the > Mika's one, so this one isn't really necessary. In case you think it > would be useful to apply it anyway, please let me know and I submit it > properly with a changelog etc. I can revert Mika's patch, as it would be good to catch these kinds of errors. > --- > drivers/pci/remove.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/pci/remove.c > =================================================================== > --- linux-pm.orig/drivers/pci/remove.c > +++ linux-pm/drivers/pci/remove.c > @@ -24,7 +24,7 @@ static void pci_stop_dev(struct pci_dev > if (dev->is_added) { > pci_proc_detach_device(dev); > pci_remove_sysfs_dev_files(dev); > - device_del(&dev->dev); > + device_release_driver(&dev->dev); > dev->is_added = 0; > } > > @@ -34,6 +34,8 @@ static void pci_stop_dev(struct pci_dev > > static void pci_destroy_dev(struct pci_dev *dev) > { > + device_del(&dev->dev); > + > down_write(&pci_bus_sem); > list_del(&dev->bus_list); > up_write(&pci_bus_sem); This looks good, thanks for finding this. I suggest resending it with a changelog so that Bjorn can get it into 3.13. thanks, greg k-h