linux-kernel - Re: SATA resume slowness, e1000 MSI warning

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <m1649cdfas.fsf@ebiederm.dsl.xmission.com>
Date:	Wed, 07 Mar 2007 12:28:11 -0700
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	"Kok, Auke" <auke-jan.h.kok@...el.com>
Cc:	Ingo Molnar <mingo@...e.hu>, Jeff Garzik <jeff@...zik.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	"Michael S. Tsirkin" <mst@...lanox.co.il>,
	Pavel Machek <pavel@....cz>,
	Jens Axboe <jens.axboe@...cle.com>,
	Adrian Bunk <bunk@...sta.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Thomas Gleixner <tglx@...utronix.de>, linux-pm@...ts.osdl.org,
	Michal Piotrowski <michal.k.k.piotrowski@...il.com>
Subject: Re: SATA resume slowness, e1000 MSI warning

"Kok, Auke" <auke-jan.h.kok@...el.com> writes:

> Kok, Auke wrote:
>> Eric W. Biederman wrote:

>>> This leaves with some basic questions.
>>> - Does it make sense for suspend/resume methods to request/free irqs?
>>> - Does it make sense for suspend/resume methods to allocate/free msi irqs?
>>> - Do we want pci_save/restore_cap to save/restore msi state?
>>>
>>> The path of least resistance is to just free the extra state and we
>>> are good.  I'm just not quite certain that is sane and it has been a
>>> long day.
>>
>> we used to have a lengthy e1000_pci_save|restore_state in our code, which is
>> now gone, so I'm all for that. A separate pci_save_pxie|msi(x)_state for every
>> driver seems completely unnecessary. I can't think of a use case where
>> saving+restoring everything hurts. That's what you want I presume.

I just want to understand why we have issues and to see if how we have
organized the suspend/resume path for dealing with msi irqs makes sense.

That is I haven't looked much at the suspend/resume path so I don't know it
well and I am afraid that your problem might be a symptom of a deeper
problem.

>> We currently free all irq's and msi before going into suspend in e1000, and I
>> think that is probably a good thing, somehow I can think of bad things
>> happening if we dont, but I admit that I haven't tried it without
>> alloc/free. We do this in e100 as well and it works.

Currently the irq code supports operation without the
free_irq/request_irq.  Since the numbers given are pure linux
abstractions things should it is really a matter of just
saving/restoring the appropriate state.

>> Another motivation would be to leave this up to the driver: if the driver
>> chooses to free/alloc interrupts because it makes sense, you probably would
>> want to keep that choice available. Devices that don't need this can skip the
>> alloc/free, but leave the choice open for others.
>
> ah, looking at the code in e1000 we do:
>
> _suspend:
> 	pci_save_state();
> 	free_irq()
>
> _resume:
> 	pci_restore_state();
> 	alloc_irq();
>
> I suppose that's not good either, and the major cause of the warning in the
> first place.

Yep. 

> Maybe I can rollback your latest patches and try to fix that mess by postponing
> the pci_save_state until after we free'd the irq's.

Below is an additional set of warnings that should help debug this.
The old code just got lucky that it triggered a warning when this happens.

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 01869b1..5113913 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -613,6 +613,7 @@ int pci_enable_msi(struct pci_dev* dev)
 		return -EINVAL;
 
 	WARN_ON(!!dev->msi_enabled);
+	WARN_ON(!hlist_empty(&dev->saved_cap_space));
 
 	/* Check whether driver already requested for MSI-X irqs */
 	if (dev->msix_enabled) {
@@ -638,6 +639,8 @@ void pci_disable_msi(struct pci_dev* dev)
 	if (!dev->msi_enabled)
 		return;
 
+	WARN_ON(!hlist_empty(&dev->saved_cap_space));
+
 	msi_set_enable(dev, 0);
 	pci_intx(dev, 1);		/* enable intx */
 	dev->msi_enabled = 0;
@@ -739,6 +742,7 @@ int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec)
 		}
 	}
 	WARN_ON(!!dev->msix_enabled);
+	WARN_ON(!hlist_empty(&dev->saved_cap_space));
 
 	/* Check whether driver already requested for MSI irq */
    	if (dev->msi_enabled) {
@@ -763,6 +767,8 @@ void pci_disable_msix(struct pci_dev* dev)
 	if (!dev->msix_enabled)
 		return;
 
+	WARN_ON(!hlist_empty(&dev->saved_cap_space));
+
 	msix_set_enable(dev, 0);
 	pci_intx(dev, 1);		/* enable intx */
 	dev->msix_enabled = 0;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bd44a48..4418839 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -677,6 +677,7 @@ pci_restore_state(struct pci_dev *dev)
 	}
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);
+	WARN_ON(!hlist_empty(&dev->saved_cap_space));
 
 	return 0;
 }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/