On Tue, Jun 07, 2005 at 04:58:00PM -0400, Adam Belay wrote:
> On Tue, Jun 07, 2005 at 12:55:52PM +0200, Karsten Keil wrote:
> > On Mon, Jun 06, 2005 at 10:50:55PM -0400, Adam Belay wrote:
> > > On Mon, Jun 06, 2005 at 05:04:07PM -0700, Linus Torvalds wrote:
> > > >
> > > > Jeff,
> > > > this looks ok, but I'll leave the decision to you. Things like this often
> > > > break.
> > > >
> > > > Andrew, maybe at least a few days in -mm to see if there's some outcry?
> > > >
> > > > Linus
> > >
> > > This patch is an improvement, but there may still be some issues.
> > > Specifically, it looks to me like the the interrupt handler remains
> > > registered. This could cause some problems when another device is sharing
> > > the interrupt because the tulip driver must read from a hardware register
> > > to determine if it triggered the interrupt. When the hardware has been
> > > physically powered off, things might not go well.
> >
> > No, I also looked into this, it is not needed for the tulip driver, it
> > detects that it has no access to the hardware (reading 0xffffffff) in the
> > interrupt functions.
>
> Hmm, doesn't look like it:
>
> > /* Let's see whether the interrupt really is for us */
> > csr5 = ioread32(ioaddr + CSR5);
> >
> > if (tp->flags & HAS_PHY_IRQ)
> > handled = phy_interrupt (dev);
> >
> > if ((csr5 & (NormalIntr|AbnormalIntr)) == 0)
> > return IRQ_RETVAL(handled);
>
> Upon reading the value 0xffffffff for csr5, the if statement would be false.
> Maybe it bails out later or something, but really why risk it? As Ben noted,
> it's easy to have race conditions between power off and interrupt handler
> routines. Also, there is a transition delay between D0 and D3. I prefer to
> play it safe and call free_irq().
Yes it test later for 0xffffffff, but you are right, freeing the IRQ would
be safer.
...
> >
> > I think restoring the PCI config should be done always, not only if the
> > device was in the up state, also powerup should be done.
> > If not you will run into problems to use the device later.
>
> Yeah, I think you're right.
>
> Also, after looking at some other netdev suspend routines, it appears that
> "netif_device_present" isn't necessary. Jeff, is this correct?
>
> Thanks,
> Adam
>
>
> How does this patch look:
Looks OK and also work on my HW.
Andrew, can you please take this updated version, it is much cleaner and
safer, because it handle the IRQ correctly.
>
> --- a/drivers/net/tulip/tulip_core.c 2005-05-27 22:06:00.000000000 -0400
> +++ b/drivers/net/tulip/tulip_core.c 2005-06-07 16:34:13.641177456 -0400
> @@ -1756,11 +1756,19 @@
> {
> struct net_device *dev = pci_get_drvdata(pdev);
>
> - if (dev && netif_running (dev) && netif_device_present (dev)) {
> - netif_device_detach (dev);
> - tulip_down (dev);
> - /* pci_power_off(pdev, -1); */
> - }
> + if (!dev)
> + return -EINVAL;
> +
> + if (netif_running(dev))
> + tulip_down(dev);
> +
> + netif_device_detach(dev);
> + pci_save_state(pdev);
> +
> + free_irq(dev->irq, dev);
> + pci_disable_device(pdev);
> + pci_set_power_state(pdev, pci_choose_state(pdev, state));
> +
> return 0;
> }
>
> @@ -1768,15 +1776,26 @@
> static int tulip_resume(struct pci_dev *pdev)
> {
> struct net_device *dev = pci_get_drvdata(pdev);
> + int retval;
>
> - if (dev && netif_running (dev) && !netif_device_present (dev)) {
> -#if 1
> - pci_enable_device (pdev);
> -#endif
> - /* pci_power_on(pdev); */
> - tulip_up (dev);
> - netif_device_attach (dev);
> + if (!dev)
> + return -EINVAL;
> +
> + pci_set_power_state(pdev, PCI_D0);
> + pci_restore_state(pdev);
> +
> + pci_enable_device(pdev);
> +
> + if ((retval = request_irq(dev->irq, &tulip_interrupt, SA_SHIRQ, dev->name, dev))) {
> + printk (KERN_ERR "tulip: request_irq failed in resume\n");
> + return retval;
> }
> +
> + netif_device_attach(dev);
> +
> + if (netif_running(dev))
> + tulip_up(dev);
> +
> return 0;
> }
>
--
Karsten Keil
SuSE Labs
ISDN development
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
