Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw
From: Ingo Molnar <mingo@kernel.org>
Date: 2013-12-19 18:33:17
Also in:
lkml, stable
* David Herrmann [off-list ref] wrote:
quoted hunk ↗ jump to hunk
+/* + * Unregister the sysfb and prevent new sysfbs from getting registered. Can be + * called from any context except recursively or from sysfb_register(). + * Used by remove_conflicting_framebuffers() and friends. + */ +void sysfb_unregister(const struct apertures_struct *apert, bool primary) +{ + mutex_lock(&sysfb_lock); + if (!IS_ERR(sysfb_dev) && sysfb_dev) { + if (sysfb_match(apert, primary)) { + platform_device_unregister(sysfb_dev); + sysfb_dev = ERR_PTR(-EALREADY); + } + } else { + /* if !sysfb_dev, set err so no new sysfb is probed later */ + sysfb_dev = ERR_PTR(-EALREADY);
Just a small detail: we can get into this 'else' branch not just with NULL, but also with IS_ERR(sysfb_dev). In that case we override whatever error code is contained in sysfb_dev and overwrite it with ERR_PTR(-EALREADY). (Probably not a big deal, because we don't actually ever seem to extract the error code from the pointer, but wanted to mention it.)
quoted hunk ↗ jump to hunk
+#ifdef CONFIG_X86_SYSFB +#include <asm/sysfb.h> +#endif
Pet peeve, this looks sexier: #ifdef CONFIG_X86_SYSFB # include <asm/sysfb.h> #endif
quoted hunk ↗ jump to hunk
@@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, } } +static void remove_conflicting_sysfb(const struct apertures_struct *apert, + bool primary) +{ + if (!apert) + return; + +#ifdef CONFIG_X86_SYSFB + sysfb_unregister(apert, primary); +#endif +}
So why not make sysfb_unregister() accept a !apert parameter (it would simply return), at which point remove_conflicting_sysfb() could be eliminated and just be replaced with a direct sysfb_unregister() call - with no #ifdefs. We only need #ifdefs for the sysfb_unregister() declaration in the .h file. At first sight this looks simpler and cleaner for the fix itself - no need for extra cleanups for this detail. Thanks, Ingo