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]
Date:	Tue, 3 Feb 2009 18:23:28 -0800 (PST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
cc:	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	pm list <linux-pm@...ts.linux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux PCI <linux-pci@...r.kernel.org>,
	Eric Sesterhenn <snakebyte@....de>
Subject: Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers



On Wed, 4 Feb 2009, Rafael J. Wysocki wrote:
> 
> Suspend to RAM is reported to break on some machines as a result of
> attempting to put one of driverless PCI devices into a low power
> state.  Avoid that by not attepmting to power manage driverless
> devices during suspend.
> 
> Fix up pci_pm_poweroff() after a previous incomplete fix for the same
> thing during hibernation.

Ok, I really don't like this patch, because:

> -static void pci_pm_default_suspend(struct pci_dev *pci_dev)
> +static void pci_pm_default_suspend(struct pci_dev *pci_dev, bool prepare)
>  {
>  	pci_pm_default_suspend_generic(pci_dev);
>  
> -	if (!pci_is_bridge(pci_dev))
> +	if (prepare && !pci_is_bridge(pci_dev))
>  		pci_prepare_to_sleep(pci_dev);
>  
>  	pci_fixup_device(pci_fixup_suspend, pci_dev);

This "helper" function really isn't helping anything at all any more. It's 
really just confusing things.

Now that was true even before this all; mostly because your naming in this 
area _really_ sucks. I mean, what the heck is the difference between 
"pci_pm_default_suspend_generic()" and "pci_pm_default_suspend()", and 
what do they do?

But you just made it worse. This trivial function that doesn't do anything 
interesting, and isn't well-named enough to actually explain what it is 
doing now became EVEN WORSE.  Now it's a trivial function that does two 
things, except it does one of those things only if the magic flag (that is 
also not helpfully named) is set.

Argh.

To make it worse, it's not at all obvious what the logic is:

> +	struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +		pci_pm_default_suspend(pci_dev, !!pm);

Whaa? This is basically totally obfuscated code both in the caller _and_ 
in the callee.

Now, it looks like this all then goes away in PATCH 7/7, so I guess I 
shouldn't complain too much, but I just don't see much point in carrying 
this broken patch around in the series, since it's then going away and 
rewritten almost immediately again.

Apart from that complaints, Acked-by: for the series. 

			Linus
--
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