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: <200906141909.46354.rjw@sisk.pl>
Date:	Sun, 14 Jun 2009 19:09:45 +0200
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	andi@...as.de
Cc:	akpm@...ux-foundation.org, e1000-devel@...ts.sourceforge.net,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Make e100 suspend handler support PCI cards lacking PM capability

On Sunday 14 June 2009, Andreas Mohr wrote:
> Hi,
> 
> On Sun, Jun 14, 2009 at 04:06:29PM +0200, Rafael J. Wysocki wrote:
> > On Sunday 14 June 2009, Andreas Mohr wrote:
> > > Hi,
> > > 
> > > On Sun, Jun 14, 2009 at 12:28:15AM +0200, Rafael J. Wysocki wrote:
> > > > On Saturday 13 June 2009, Andreas Mohr wrote:
> > > > > +
> > > > >  	if (wake) {
> > > > >  		return pci_prepare_to_sleep(pdev);
> > > > 
> > > > pci_prepare_to_sleep() is supposed to return 0 for your device.  I'll have a
> > > > look at it.
> > > 
> > > No, wake is false for my card, thus that's not the branch to
> > > investigate.
> > 
> > Ah.  The problem is, then, that we try to put the device into D3, which it
> > cannot do and error code is correctly returned from pci_set_power_state().
> > 
> > I would use the appended patch in that case and the patch sent previously
> > is necessary for the 'wake = true' case.
> 
> OK, as said I cannot test this right now, but I'm _damn_ sure it would
> work. Thus I'd say your equivalent patch posted a bit later can be
> committed already.
> 
> But what about the wake = true case?
> I'm not affected by this since my card doesn't have any wake capa,
> thus it's your call of whether that pci core code part really was broken
> and needed fixing.

I think it needs fixing.

> So, for the patch in your next mail:
> Acked-by: Andreas Mohr <andi@...as.de>
> 
> 
> BTW, that patch was (pasted):
> 
>  static int __e100_power_off(struct pci_dev *pdev, bool wake)
>  {
> -       if (wake) {
> +       if (wake)
>                 return pci_prepare_to_sleep(pdev);
> -       } else {
> -               pci_wake_from_d3(pdev, false);
> -               return pci_set_power_state(pdev, PCI_D3hot);
> -       }
> +
> +       pci_wake_from_d3(pdev, false);
> +       pci_set_power_state(pdev, PCI_D3hot);
> +
> +       return 0;
>  }
> 
> 
> Couple questions still:
> - why do we call pci_wake_from_d3(...false) only!?
>   Wouldn't this break WoL after one iteration back and forth,
>   due to missing 'true' case?

The 'true' case is the 'wake = true' one.

> - why do we call netif_device_detach() _after_ doing hardware shutdown
>   of the network controller? I'd guess this can cause huge issues?
>   Someone told me he had rtnl lock issues upon S2D with e100
>   (very similar to my rtnl issues during aborted .suspend),
>   and that might possibly be the reason?

I think you're right, but I'm not a network driver expert.

Perhaps you can change the ordering and see if that fixes the rtnl issue
(since you're able to reproduce it without my patch, that should be easy to
verify).

Best,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ