linux-kernel - Re: [PATCH 1/7] PCI PM: Fix handling of devices without drivers

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: <alpine.LFD.2.00.0902031815550.3247@localhost.localdomain>
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/