Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw

Hi

On Thu, Dec 19, 2013 at 7:33 PM, Ingo Molnar [off-list ref] wrote:

* David Herrmann [off-list ref] wrote:

quoted

+/*
+ * 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.)

Yepp, I know, but that's just fine.

quoted

+#ifdef CONFIG_X86_SYSFB
+#include <asm/sysfb.h>
+#endif

Pet peeve, this looks sexier:

#ifdef CONFIG_X86_SYSFB
# include <asm/sysfb.h>
#endif

quoted

@@ -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.

I can do that but we still need the #ifdef. sysfb is an x86-asm header
so it's not available on other archs. Even with #ifdef in the header I
still need it around the actual function call.

Thanks
David

At first sight this looks simpler and cleaner for the fix itself - no
need for extra cleanups for this detail.

Thanks,

        Ingo