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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
