| Message ID | 1387367283-20078-1-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* David Herrmann <dh.herrmann@gmail.com> 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@gmail.com> > --- > 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.. Please fix the changelog to conform to the standard changelog style: - first describe the symptoms of the bug - how does a user notice? - then describe how the code behaves today and how that is causing the bug - and then only describe how it's fixed. The first item is the most important one - while developers (naturally) tend to concentrate on the least important point, the last one. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@gmail.com> > --- > 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? 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: 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; } } > + > +__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. thanks, Takashi -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi On Wed, Dec 18, 2013 at 2:03 PM, Takashi Iwai <tiwai@suse.de> 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@gmail.com> >> --- >> 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-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@suse.de> 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@gmail.com> > >> --- > >> 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-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; +} + +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); +} + +__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; + +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 +} + static int do_register_framebuffer(struct fb_info *fb_info) { int i; @@ -1742,6 +1753,8 @@ EXPORT_SYMBOL(unlink_framebuffer); void remove_conflicting_framebuffers(struct apertures_struct *a, const char *name, bool primary) { + remove_conflicting_sysfb(a, primary); + mutex_lock(®istration_lock); do_remove_conflicting_framebuffers(a, name, primary); mutex_unlock(®istration_lock); @@ -1762,6 +1775,9 @@ register_framebuffer(struct fb_info *fb_info) { int ret; + remove_conflicting_sysfb(fb_info->apertures, + fb_is_primary_device(fb_info)); + mutex_lock(®istration_lock); ret = do_register_framebuffer(fb_info); mutex_unlock(®istration_lock);
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@gmail.com> --- 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(-)