From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758230Ab0JLTqE (ORCPT ); Tue, 12 Oct 2010 15:46:04 -0400 Received: from cpoproxy2-pub.bluehost.com ([67.222.39.38]:39576 "HELO cpoproxy2-pub.bluehost.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1757821Ab0JLTqC (ORCPT ); Tue, 12 Oct 2010 15:46:02 -0400 DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=virtuousgeek.org; h=Received:Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References:X-Mailer:Mime-Version:Content-Type:Content-Transfer-Encoding:X-Identified-User; b=I57dkazTSvtVZCDhrDHI6LbsrsKa7cZz+6qDTd9J+Edz7L7/TUn/I61P8yXPpw1GwFn3SSIofsWczHsexZ74swlA9l0YiAQrze+0vTOKaAMzj8STi9ptvKUAOlnifvOR; Date: Tue, 12 Oct 2010 12:49:38 -0700 From: Jesse Barnes To: "Rafael J. Wysocki" Cc: Dave Airlie , Thomas Gleixner , LKML , Ingo Molnar , Keith Packard Subject: Re: "do_IRQ: 0.89 No irq handler for vector (irq -1)" Message-ID: <20101012124938.47170457@jbarnes-desktop> In-Reply-To: <201010122101.17812.rjw@sisk.pl> References: <20101011154826.440b4990@jbarnes-desktop> <20101011164014.5a11ee48@jbarnes-desktop> <201010122101.17812.rjw@sisk.pl> X-Mailer: Claws Mail 3.7.6 (GTK+ 2.18.9; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Identified-User: {10642:box514.bluehost.com:virtuous:virtuousgeek.org} {sentby:smtp auth 67.174.193.198 authed with jbarnes@virtuousgeek.org} Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 12 Oct 2010 21:01:17 +0200 "Rafael J. Wysocki" wrote: > On Tuesday, October 12, 2010, Jesse Barnes wrote: > > On Mon, 11 Oct 2010 15:48:26 -0700 > > Jesse Barnes wrote: > > > > > On Fri, 8 Oct 2010 21:46:50 +1000 > > > Dave Airlie wrote: > > > > Not sure how best to fix, I can workaround by calling > > > > pci_set_power_state(PCI_D0) in the drm drivers, but I sorta thing the > > > > PCI layer should take care of this. > > > > > > So I think we *should* be able to call pci_disable_device at remove > > > time. But as you say, some platforms may not correctly re-route VGA > > > space to an existing device or disable it properly when we do that. > > > AFAICT x86 will be ok here though (seems to work ok locally too). > > > > Just tested this some more, and I think it's the right thing to do in > > the KMS case at least. When we load a KMS driver it takes over the gfx > > device and nothing can assume anything about VGA state unless using the > > VGA arbiter. So calling pci_disable_device() in the shutdown path of a > > KMS driver shouldn't make things any worse, and will work around this > > issue. > > > > Doing so in the non-KMS case violates some PC assumptions though, in > > that things like vgacon and the BIOS will assume VGA memory is still > > around, which on some platforms pci_disable_device() may affect (I only > > checked the x86 implementation). > > > > > That said, it seems like we should update the current device state at > > > load time as well, once we've matched the driver it seems like there > > > should be no harm. > > > > > > Rafael, what do you think? Would having the correct power state at > > > load time cause any trouble with other PM code? I know we've had > > > issues with setting it explicitly in the past... > > > > So we should probably make pci_enable_device pick up the current state > > as well, instead of assuming it's unknown just because the enable count > > was non-zero (which as Dave points out, can be affected by sysfs writes > > too). > > > > The only downside I can think of there is that if the device is already > > enabled, we generally have to assume another driver owns it, and who > > knows if the device is actually alive enough to read the current state > > from. But I think we handle those errors ok too, so pulling it out > > should be safe. > > I remember trying to do something like this and it didn't play well with the > initialization. Still, I didn't do that in pci_enable_device(), so I can't say > for sure at the moment. I _think_ it will be fine, though. Seems to work ok for the buggy i915 reload case. But looking at it again, I really don't like two things: 1) doing just the set_power_state seems wrong, what about the rest of enable? 2) allowing nested enables at all I know sysfs currently allows us to bump the enable count to arbitrary levels, but that's easy to fix; we can just check pci_is_enabled() before calling enable_device in the sysfs store routine. If we did that, we could warn when pci_enable_device is called in a nested way, which is generally a bug (two drivers trying to take over a device?). I don't think I buy that VGA is special anyway, at least not for KMS enabled kernels, where vgacon and the BIOS can't assume anything about graphics state anymore. More generally, I don't think BIOSes have been able to assume anything about the current graphics state since Windows 3.1, when most platforms stopped using BIOS calls and/or VGA regs for mode setting. Thoughts? -- Jesse Barnes, Intel Open Source Technology Center --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -963,9 +963,6 @@ static int do_pci_enable_device(struct pci_dev *dev, int bar { int err; - err = pci_set_power_state(dev, PCI_D0); - if (err < 0 && err != -EIO) - return err; err = pcibios_enable_device(dev, bars); if (err < 0) return err; @@ -994,6 +991,10 @@ static int __pci_enable_device_flags(struct pci_dev *dev, int err; int i, bars = 0; + err = pci_set_power_state(dev, PCI_D0); + if (err < 0 && err != -EIO) + return err; + if (atomic_add_return(1, &dev->enable_cnt) > 1) return 0; /* already enabled */