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

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: <20131219183310.GF32508@gmail.com>
Date:	Thu, 19 Dec 2013 19:33:10 +0100
From:	Ingo Molnar <mingo@...nel.org>
To:	David Herrmann <dh.herrmann@...il.com>
Cc:	linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>, x86@...nel.org,
	linux-fbdev@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw


* David Herrmann <dh.herrmann@...il.com> wrote:

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

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

Pet peeve, this looks sexier:

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

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