[Intel-gfx] [PATCH 10/10] drm/i915: Disable use of stolen area by User when Intel RST is present

[Lukas Wunner]

Hi,

On Thu, Feb 04, 2016 at 04:05:04PM +0000, Chris Wilson wrote:
> On Thu, Feb 04, 2016 at 04:46:55PM +0100, Lukas Wunner wrote:
> > > +	/* If the stolen region can be modified behind our backs upon suspend,
> > > +	 * then we cannot use it to store nonvolatile contents (i.e user data)
> > > +	 * as it will be corrupted upon resume.
> > > +	 */
> > > +	dev_priv->mm.nonvolatile_stolen = true;
> > > +#ifdef CONFIG_SUSPEND
> > > +	if (intel_detect_acpi_rst()) {
> > > +		/* BIOSes using RapidStart Technology have been reported
> > > +		 * to overwrite stolen across S3, not just S4.
> > > +		 */
> > > +		dev_priv->mm.nonvolatile_stolen = false;
> > > +	}
> > > +#endif
> > > +
> > 
> > I'd suggest simplifying it like this:
> > 
> > 	dev_priv->mm.nonvolatile_stolen = !(IS_ENABLED(CONFIG_SUSPEND) &&
> > 					    acpi_dev_present("INT3392"));
> 
> Using if (IS_ENABLED(CONFIG_SUSPEND) && intel_detect_acpi_rst()) would
> be better indeed. My main concern here is that we document carefully why
> we had to disable this, and to leave room for future caveats.

Yes absolutely, keep the comments. (Or meld them into one if simplifying
the code as suggested.)

> We could #define INTEL_RAPID_START "INT3392" for
> if (IS_ENABLED(CONFIG_SUSPEND) && acpi_dev_present(INTEL_RAPID_START))
> but Anki wanted to keep the acpi details themselves out of stolen (hence
> the current split).

At the very least the #ifdef needs to be replaced by IS_ENABLED,
Documentation/CodingStyle chapter 20 very clearly states this is
to be preferred.

Less code is almost always better, it's more work for a reader to
follow the logic if things are split across multiple files.

If you absolutely positively want to keep the current split,
the "static const struct acpi_device_id irst_ids[]" data structure
should be replaced by a "static const char*" in order to not waste
memory.

I'd also suggest renaming "dev_priv->mm.nonvolatile_stolen" to
"volatile_stolen" since the only user of the flag uses its negation
and its calculation requires negation as well.

Best regards,

Lukas