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

From: Takashi Iwai
Date: Wed Dec 18 2013 - 09:02:25 EST

At Wed, 18 Dec 2013 14:34:53 +0100,
David Herrmann wrote:
>
> 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.

OK, but please add a comment on it (that it can be called outside fb
lock).


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

Fair enough. But this would be better to be commented there, too, for
avoiding further stray sheep :)


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

OK, I'll test it soon.


thanks!

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