Re: [RFC] x86: sysfb: remove sysfb when probing real hw

[David Herrmann]

Hi

On Wed, Dec 18, 2013 at 2:03 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> At Wed, 18 Dec 2013 12:48:03 +0100,
> David Herrmann wrote:
>>
>> If we probe a real hw driver for graphics devices, we need to unload any
>> generic fallback driver like efifb/vesafb/simplefb on the system
>> framebuffer. This is currently done via remove_conflicting_framebuffers()
>> in fbmem.c. However, this only removes the fbdev driver, not the fake
>> platform devices underneath. This hasn't been a problem so far, as efifb
>> and vesafb didn't store any resources there. However, with simplefb this
>> changed.
>>
>> To correctly release the IORESOURCE_MEM resources of simple-framebuffer
>> platform devices, we need to unregister the underlying platform device
>> *before* probing any new hw driver. This patch adds sysfb_unregister() for
>> that purpose. It can be called from any context (except from the
>> platform-device ->remove callback path) and synchronously unloads any
>> global sysfb and prevents new sysfbs from getting registered. Thus, you
>> can call it even before any sysfb has been loaded.
>>
>> This also changes remove_conflicting_framebuffer() to call this helper
>> *before* trying it's fbdev heuristic to remove conflicting drivers.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
>> ---
>> Hi
>>
>> This is imho the clean version of Takashi's fix. However, it gets pretty huge. I
>> wouldn't object to marking CONFIG_X86_SYSFB broken in the stable series and get
>> this in for 3.14. Your call..
>>
>> This patch basically simulates an unplug event for system-framebuffers when
>> loading real hardware drivers. To trigger it, call sysfb_unregister(). You can
>> optionally pass an aperture-struct and primary-flag similar to
>> remove_conflicting_framebuffers(). If they're not passed, we remove it
>> unconditionally.
>>
>> Untested, but my kernel compiles are already running. If my tests succeed and
>> nobody has objections, I can resend it as proper PATCH and marked for stable.
>> And maybe split the fbmem and sysfb changes into two patches..
>>
>> Thanks
>> David
>>
>>  arch/x86/include/asm/sysfb.h     |  10 ++++
>>  arch/x86/kernel/sysfb.c          | 103 +++++++++++++++++++++++++++++++++++++--
>>  arch/x86/kernel/sysfb_simplefb.c |   5 +-
>>  drivers/video/fbmem.c            |  16 ++++++
>>  4 files changed, 128 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
>> index 2aeb3e2..713bc17 100644
>> --- a/arch/x86/include/asm/sysfb.h
>> +++ b/arch/x86/include/asm/sysfb.h
>> @@ -11,6 +11,7 @@
>>   * any later version.
>>   */
>>
>> +#include <linux/fb.h>
>>  #include <linux/kernel.h>
>>  #include <linux/platform_data/simplefb.h>
>>  #include <linux/screen_info.h>
>> @@ -59,6 +60,15 @@ struct efifb_dmi_info {
>>       int flags;
>>  };
>>
>> +__init struct platform_device *sysfb_alloc(const char *name,
>> +                                        int id,
>> +                                        const struct resource *res,
>> +                                        unsigned int res_num,
>> +                                        const void *data,
>> +                                        size_t data_size);
>> +__init int sysfb_register(struct platform_device *dev);
>> +void sysfb_unregister(const struct apertures_struct *apert, bool primary);
>> +
>>  #ifdef CONFIG_EFI
>>
>>  extern struct efifb_dmi_info efifb_dmi_list[];
>> diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
>> index 193ec2c..3d4554e 100644
>> --- a/arch/x86/kernel/sysfb.c
>> +++ b/arch/x86/kernel/sysfb.c
>> @@ -33,11 +33,106 @@
>>  #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;
>> +
>> +/* register @dev as sysfb; takes ownership over @dev */
>> +__init int sysfb_register(struct platform_device *dev)
>> +{
>> +     int r = 0;
>> +
>> +     mutex_lock(&sysfb_lock);
>> +     if (!sysfb_dev) {
>> +             r = platform_device_add(dev);
>> +             if (r < 0)
>> +                     put_device(&dev->dev);
>> +             else
>> +                     sysfb_dev = dev;
>> +     } else {
>> +             /* if there already is/was a sysfb, destroy @pd but return 0 */
>> +             platform_device_put(dev);
>> +     }
>> +     mutex_unlock(&sysfb_lock);
>> +
>> +     return r;
>> +}
>
>
> Since sysfb_alloc() always follows sysfb_register() and they are
> always coupled, we can simply combine both to one?

We actually cannot call sysfb_register() for efi/vesa-framebuffer as
they lack a ->remove() callback. I fixed that in v2. I will send a
patch which adds ->remove() callbacks for vesafb and efifb later, but
this shouldn't go into this fix.
Furthermore, I think splitting them makes them easier to read.

> Also, do we really need a mutex?  The functions in fbmem.c are already
> in registeration_lock, so if this is called only from there, it should
> be fine without an extra lock here.  So, the function can be
> simplified like:

They have to be called outside of the fb-mutex. We trigger the
->remove() callback, which will call unregister_framebuffer() which
will lock the fb-mutex => deadlock. And as
remove_conflicting_framebuffers() can be called in parallel by many
drivers, we need the lock in sysfb. We could use an
atomic_test_and_dec(), but that might cause one removal to overtake
the previous unregistration. Thus I added the mutex.

Also note that with CONFIG_FB=n and the pending SimpleDRM driver, we
will call sysfb_unregister() from outside of fbmem.c. If we depend on
fbmem.c internal locks, we need to change it for 3.14/15 again.

> int 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 *pdev;
>         if (sysfb_dev)
>                 return ret;
>         pdev = platform_device_register_resndata(....);
>         if (IS_ERR(pdev))
>                 return PTR_ERR(pdev);
>         sysfb_dev = pdev;
>         return 0;
> }
>
>
>> +
>> +static bool sysfb_match(const struct apertures_struct *apert, bool primary)
>> +{
>> +     struct screen_info *si = &screen_info;
>> +     unsigned int i;
>> +     const struct aperture *a;
>> +
>> +     if (!apert || primary)
>> +             return true;
>> +
>> +     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;
>> +}
>> +
>> +/* unregister the sysfb and prevent new sysfbs from getting registered */
>> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
>> +{
>> +
>> +     mutex_lock(&sysfb_lock);
>> +     if (!IS_ERR(sysfb_dev)) {
>> +             if (sysfb_dev) {
>> +                     if (sysfb_match(apert, primary)) {
>> +                             platform_device_del(sysfb_dev);
>> +                             platform_device_put(sysfb_dev);
>> +                             sysfb_dev = ERR_PTR(-EALREADY);
>> +                     }
>> +             } else {
>> +                     sysfb_dev = ERR_PTR(-EALREADY);
>> +             }
>> +     }
>> +     mutex_unlock(&sysfb_lock);
>> +}
>
> Simpler would be like:
>
> void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> {
>         if (sysfb_dev && sysfb_match(apert, primary)) {
>                 platform_device_unregister(sysfb_dev);
>                 sysfb_dev = NULL;
>         }
> }

Nope, if sysfb_dev is NULL, I need to set it to ERR_PTR(-sth).
Otherwise, imagine i915 getting loaded before sysfb_init(). It calls
sysfb_unregister() without a registered sysfb and continues. A later
sysfb_init() will then load sysfb anyway. With my code, this is
prevented by setting sysfb_dev to ERR_PTR(-EALREADY).

Maybe the device-init ordering prevents this, but I'm not entirely
sure about this so lets be safe and have a strict ordering.

>
>> +
>> +__init struct platform_device *sysfb_alloc(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;
>> +
>> +     pd = platform_device_alloc(name, id);
>> +     if (!pd)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     ret = platform_device_add_resources(pd, res, res_num);
>> +     if (ret)
>> +             goto err;
>> +
>> +     ret = platform_device_add_data(pd, data, data_size);
>> +     if (ret)
>> +             goto err;
>> +
>> +     return pd;
>
> I don't think we need to open-code this if we can use
> platform_device_register_*() helper.
>
>
>> +
>> +err:
>> +     platform_device_put(pd);
>> +     return ERR_PTR(ret);
>> +}
>> +
>>  static __init int sysfb_init(void)
>>  {
>>       struct screen_info *si = &screen_info;
>> @@ -65,9 +160,11 @@ static __init int sysfb_init(void)
>>       else
>>               name = "platform-framebuffer";
>>
>> -     pd = platform_device_register_resndata(NULL, name, 0,
>> -                                            NULL, 0, si, sizeof(*si));
>> -     return IS_ERR(pd) ? PTR_ERR(pd) : 0;
>> +     pd = sysfb_alloc(name, 0, NULL, 0, si, sizeof(*si));
>> +     if (IS_ERR(pd))
>> +             return PTR_ERR(pd);
>> +
>> +     return sysfb_register(pd);
>>  }
>>
>>  /* must execute after PCI subsystem for EFI quirks */
>> diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
>> index 86179d4..8e7bd23 100644
>> --- a/arch/x86/kernel/sysfb_simplefb.c
>> +++ b/arch/x86/kernel/sysfb_simplefb.c
>> @@ -86,10 +86,9 @@ __init int create_simplefb(const struct screen_info *si,
>>       if (res.end <= res.start)
>>               return -EINVAL;
>>
>> -     pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
>> -                                            &res, 1, mode, sizeof(*mode));
>> +     pd = sysfb_alloc("simple-framebuffer", 0, &res, 1, mode, sizeof(*mode));
>>       if (IS_ERR(pd))
>>               return PTR_ERR(pd);
>>
>> -     return 0;
>> +     return sysfb_register(pd);
>>  }
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index 010d191..53e3894 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -35,6 +35,9 @@
>>
>>  #include <asm/fb.h>
>>
>> +#if IS_ENABLED(CONFIG_X86_SYSFB)
>> +#include <asm/sysfb.h>
>> +#endif
>>
>>      /*
>>       *  Frame buffer device initialization and setup routines
>> @@ -1604,6 +1607,14 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>>       }
>>  }
>>
>> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
>> +                                  bool primary)
>> +{
>> +#if IS_ENABLED(CONFIG_X86_SYSFB)
>> +     sysfb_unregister(apert, primary);
>> +#endif
>
> I noticed that sysfb.c is built even without CONFIG_X86_SYSFB.
> So this can be called even for non-sysfb case (which is also good to
> release the unused platform_device).
>
> That is, this (and the above) can be #ifdef CONFIG_X86 instead.

Yepp, fixed.

I will send v2 in a minute. I tested it on x86 with efifb+no-sysfb and
simplefb+sysfb, both handovers worked fine.

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/