Re: [PATCH 02/11] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

Hi

On Thu, Jan 23, 2014 at 5:51 PM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> Just a couple of small nits:
>
> * David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
>
>> --- a/arch/x86/kernel/sysfb.c
>> +++ b/arch/x86/kernel/sysfb.c
>> @@ -33,11 +33,76 @@
>>  #include <linux/init.h>
>>  #include <linux/kernel.h>
>>  #include <linux/mm.h>
>> +#include <linux/mutex.h>
>>  #include <linux/platform_data/simplefb.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/screen_info.h>
>>  #include <asm/sysfb.h>
>>
>> +static DEFINE_MUTEX(sysfb_lock);
>> +static struct platform_device *sysfb_dev;
>> +
>> +int __init sysfb_register(const char *name, int id,
>> +                       const struct resource *res, unsigned int res_num,
>> +                       const void *data, size_t data_size)
>> +{
>> +     struct platform_device *pd;
>> +     int ret = 0;
>> +
>> +     mutex_lock(&sysfb_lock);
>> +     if (!sysfb_dev) {
>> +             pd = platform_device_register_resndata(NULL, name, id,
>> +                                                    res, res_num,
>> +                                                    data, data_size);
>> +             if (IS_ERR(pd))
>> +                     ret = PTR_ERR(pd);
>> +             else
>> +                     sysfb_dev = pd;
>> +     }
>> +     mutex_unlock(&sysfb_lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static bool sysfb_match(const struct apertures_struct *apert)
>> +{
>> +     struct screen_info *si = &screen_info;
>> +     unsigned int i;
>> +     const struct aperture *a;
>> +
>> +     for (i = 0; i < apert->count; ++i) {
>> +             a = &apert->ranges[i];
>> +             if (a->base >= si->lfb_base &&
>> +                 a->base < si->lfb_base + ((u64)si->lfb_size << 16))
>> +                     return true;
>> +             if (si->lfb_base >= a->base &&
>> +                 si->lfb_base < a->base + a->size)
>> +                     return true;
>> +     }
>> +
>> +     return false;
>> +}
>> +
>> +/* Remove sysfb and disallow new sysfbs from now on. Can be called from any
>> + * context except recursively (see also remove_conflicting_framebuffers()). */
>> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
>
> Please use the customary (multi-line) comment style:
>
>   /*
>    * Comment .....
>    * ...... goes here.
>    */
>
> specified in Documentation/CodingStyle.

Whoops, will fix it up. Still used to that from HID code.

>> +#ifdef CONFIG_X86_SYSFB
>> +#  include <asm/sysfb.h>
>> +#endif
>
> I guess a single space is sufficient?
>
> Better yet, I'd include sysfb.h unconditionally:

Unconditionally won't work as only x86 has this header. If there's a
way to place a dummy into asm-generic which is picked if
arch/xy/include/asm/ doesn't have the header, let me know. But if I
include it unconditionally without any fallback, this will fail on
non-x86.
And adding the header to all archs seems overkill.

>> @@ -1773,6 +1780,10 @@ register_framebuffer(struct fb_info *fb_info)
>>  {
>>       int ret;
>>
>> +#ifdef CONFIG_X86_SYSFB
>> +     sysfb_unregister(fb_info->apertures, fb_is_primary_device(fb_info));
>> +#endif
>
> So, if a dummy sysfb_unregister() inline was defined in the
> !CONFIG_X86_SYSFB case then this ugly #ifdef could possibly be
> removed? Especially as it's used twice.

Again, this is fine for x86, but not for other archs. I would still
need the #ifdef x86.
Note that patch #6 introduces linux/sysfb.h and removes all these ugly
#ifdefs again. They're only needed to fix the x86 code *now*. Patch #6
generalizes the x86-sysfb infrastructure and makes it
arch-independent. But Patch #6 introduces new features and thus
shouldn't go to stable or 3.14.

As Patch #1 already fixes nearly all issues with sysfb, let me know if
you want to drop this patch and just wait for the arch-independent
sysfb to get merged. This patch is only needed if people enable
X86_SYSFB *and* FB_SIMPLE *purposely* and want hw-handover. The case
were people enable it accidentally is fixed by Patch #1.
The situation is kind of screwed.. sorry for that.

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/