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] [day] [month] [year] [list]

Date:	Fri, 20 Dec 2013 15:46:20 +0100
From:	David Herrmann <dh.herrmann@...il.com>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"the arch/x86 maintainers" <x86@...nel.org>,
	"linux-fbdev@...r.kernel.org" <linux-fbdev@...r.kernel.org>,
	stable <stable@...r.kernel.org>
Subject: Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw

Hi

On Thu, Dec 19, 2013 at 7:33 PM, Ingo Molnar <mingo@...nel.org> wrote:
>
> * 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.)

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

>> +#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.

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