| 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
| ||
|
Date: Fri, 30 Jan 2009 01:10:55 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: "Matt Carlson" <mcarlson@...adcom.com>
Cc: "Parag Warudkar" <parag.lkml@...il.com>,
"Linus Torvalds" <torvalds@...ux-foundation.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
"David S. Miller" <davem@...emloft.net>,
"Andrew Morton" <akpm@...ux-foundation.org>
Subject: Re: 2.6.29-rc3: tg3 dead after resume
On Friday 30 January 2009, Matt Carlson wrote:
> On Thu, Jan 29, 2009 at 03:03:37PM -0800, Rafael J. Wysocki wrote:
> > On Thursday 29 January 2009, Parag Warudkar wrote:
> > >
> > > On Thu, 29 Jan 2009, Matt Carlson wrote:
> > >
> > > > Can you apply the following test patch and see if it helps? The patch
> > > > does two things. First, it enables a bit which should restore firmware
> > > > communication. If that fixes the problem, then let me know and I'll
> > > > spin a proper patch.
> > > >
> > > > In the event that it doesn't work, the patch goes on to test the memory
> > > > mapping by simply printing the register value at offset 0x0. The value
> > > > should be the device's vendor ID and device ID. Please post the
> > > > results so that I can verify it.
> > > >
> > > >
> > > > diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> > > > index 8b3f846..39fce42 100644
> > > > --- a/drivers/net/tg3.c
> > > > +++ b/drivers/net/tg3.c
> > > > @@ -7227,6 +7227,11 @@ static int tg3_init_hw(struct tg3 *tp, int reset_phy)
> > > > {
> > > > tg3_switch_clocks(tp);
> > > >
> > > > + printk( KERN_NOTICE "%s: Reg value at offset 0x0 is 0x%x\n",
> > > > + tp->dev->name, tr32(0x0) );
> > > > +
> > > > + tw32(MEMARB_MODE, tr32(MEMARB_MODE) | MEMARB_MODE_ENABLE);
> > > > +
> > > > tw32(TG3PCI_MEM_WIN_BASE_ADDR, 0);
> > > >
> > > > return tg3_reset_hw(tp, reset_phy);
> > > >
> > >
> > > Hi Matt,
> > >
> > > Thanks for the patch. It didn't help with resume - but below is the
> > > output after patching, let me know if you need more details.
> >
> > [--snip--]
> >
> > In the meantime I tried to rework tg3 suspend/resume so that it uses the new
> > PCI core capability of handling the PCI-specific parts of both operations.
> >
> > The patch is appended, please see if it makes any difference.
> >
> > Thanks,
> > Rafael
> >
> > ---
> > drivers/net/tg3.c | 70 +++++++++++++++++++-----------------------------------
> > 1 file changed, 25 insertions(+), 45 deletions(-)
> >
> > Index: linux-2.6/drivers/net/tg3.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/net/tg3.c
> > +++ linux-2.6/drivers/net/tg3.c
> > @@ -13330,18 +13330,13 @@ static void __devexit tg3_remove_one(str
> > }
> > }
> >
> > -static int tg3_suspend(struct pci_dev *pdev, pm_message_t state)
> > +#ifdef CONFIG_PM
> > +
> > +static int tg3_suspend(struct device *device)
> > {
> > + struct pci_dev *pdev = to_pci_dev(device);
> > struct net_device *dev = pci_get_drvdata(pdev);
> > struct tg3 *tp = netdev_priv(dev);
> > - pci_power_t target_state;
> > - int err;
> > -
> > - /* PCI register 4 needs to be saved whether netif_running() or not.
> > - * MSI address and data need to be saved if using MSI and
> > - * netif_running().
> > - */
> > - pci_save_state(pdev);
> >
> > if (!netif_running(dev))
> > return 0;
> > @@ -13363,50 +13358,19 @@ static int tg3_suspend(struct pci_dev *p
> > tp->tg3_flags &= ~TG3_FLAG_INIT_COMPLETE;
> > tg3_full_unlock(tp);
> >
> > - target_state = pdev->pm_cap ? pci_target_state(pdev) : PCI_D3hot;
> > -
> > - err = tg3_set_power_state(tp, target_state);
>
> tg3_set_power_state() does way more than configuring the power
> management registers to the desired state though. It sets up WOL,
> configures the chip clocks, etc. This isn't safe to remove.
OK, so it requires more care to be taken.
However, suspend-resume seems to work on my test box with this patch applied,
although admittedly I haven't tested WoL.
I still am interested if it makes any difference for Parag.
> > - if (err) {
> > - int err2;
> > -
> > - tg3_full_lock(tp, 0);
> > -
> > - tp->tg3_flags |= TG3_FLAG_INIT_COMPLETE;
> > - err2 = tg3_restart_hw(tp, 1);
> > - if (err2)
> > - goto out;
> > -
> > - tp->timer.expires = jiffies + tp->timer_offset;
> > - add_timer(&tp->timer);
> > -
> > - netif_device_attach(dev);
> > - tg3_netif_start(tp);
> > -
> > -out:
> > - tg3_full_unlock(tp);
> > -
> > - if (!err2)
> > - tg3_phy_start(tp);
> > - }
> > -
> > - return err;
> > + return 0;
> > }
> >
> > -static int tg3_resume(struct pci_dev *pdev)
> > +static int tg3_resume(struct device *device)
> > {
> > + struct pci_dev *pdev = to_pci_dev(device);
> > struct net_device *dev = pci_get_drvdata(pdev);
> > struct tg3 *tp = netdev_priv(dev);
> > int err;
> >
> > - pci_restore_state(tp->pdev);
> > -
> > if (!netif_running(dev))
> > return 0;
> >
> > - err = tg3_set_power_state(tp, PCI_D0);
>
> ...and here tg3_set_power_state() restores our ability to communicate
> with the chip via MMIO.
If that were the case, it wouldn't work after a resume with the patch applied.
Still, it does work, at least with the chip I have here.
> Also, after restoring the power state to D0, the chip is switched back from
> VAux to VMain.
Are you sure this actually happens? On my box the chip is in D0 already
when the BIOS returns control to the kernel.
> This isn't safe either.
Anyway, I'd very much prefer to separate the generic PCI operations from the
device-specific code.
Thanks,
Rafael
--
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/
Powered by blists - more mailing lists