Message ID | 20220511113039.1252432-1-javierm@redhat.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | Fix some races between sysfb device registration and drivers probe | expand |
Hi Javier Am 11.05.22 um 13:30 schrieb Javier Martinez Canillas: > Drivers that want to remove registered conflicting framebuffers prior to > register their own framebuffer, calls remove_conflicting_framebuffers(). > > This function takes the registration_lock mutex, to prevent a races when > drivers register framebuffer devices. But if a conflicting framebuffer > device is found, the underlaying platform device is unregistered and this > will lead to the platform driver .remove callback to be called, which in > turn will call to the unregister_framebuffer() that takes the same lock. > > To prevent this, a struct fb_info.forced_out field was used as indication > to unregister_framebuffer() whether the mutex has to be grabbed or not. > > A cleaner solution is to drop the lock before platform_device_unregister() > so unregister_framebuffer() can take it when called from the fbdev driver, > and just grab the lock again after the device has been registered and do > a removal loop restart. > > Since the framebuffer devices will already be removed, the loop would just > finish when no more conflicting framebuffers are found. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> I'd like to shrink this patchset. This looks like it can be merged immediately? Best regards Thomas > --- > > (no changes since v1) > > drivers/video/fbdev/core/fbmem.c | 22 +++++++++++++++------- > include/linux/fb.h | 1 - > 2 files changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index b445a7a00def..2fda5917c212 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -1555,6 +1555,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, > { > int i; > > +restart_removal: > /* check all firmware fbs and kick off if the base addr overlaps */ > for_each_registered_fb(i) { > struct apertures_struct *gen_aper; > @@ -1587,12 +1588,23 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, > pr_warn("fb%d: no device set\n", i); > do_unregister_framebuffer(registered_fb[i]); > } else if (dev_is_platform(device)) { > - registered_fb[i]->forced_out = true; > + /* > + * Drop the lock because if the device is unregistered, its > + * driver will call to unregister_framebuffer(), that takes > + * this lock. > + */ > + mutex_unlock(®istration_lock); > platform_device_unregister(to_platform_device(device)); > + mutex_lock(®istration_lock); > } else { > pr_warn("fb%d: cannot remove device\n", i); > do_unregister_framebuffer(registered_fb[i]); > } > + /* > + * Restart the removal loop now that the device has been > + * unregistered and its associated framebuffer gone. > + */ > + goto restart_removal; > } > } > } > @@ -1899,13 +1911,9 @@ EXPORT_SYMBOL(register_framebuffer); > void > unregister_framebuffer(struct fb_info *fb_info) > { > - bool forced_out = fb_info->forced_out; > - > - if (!forced_out) > - mutex_lock(®istration_lock); > + mutex_lock(®istration_lock); > do_unregister_framebuffer(fb_info); > - if (!forced_out) > - mutex_unlock(®istration_lock); > + mutex_unlock(®istration_lock); > } > EXPORT_SYMBOL(unregister_framebuffer); > > diff --git a/include/linux/fb.h b/include/linux/fb.h > index 69c67c70fa78..bbe1e4571899 100644 > --- a/include/linux/fb.h > +++ b/include/linux/fb.h > @@ -511,7 +511,6 @@ struct fb_info { > } *apertures; > > bool skip_vt_switch; /* no VT switch on suspend/resume required */ > - bool forced_out; /* set when being removed by another driver */ > }; > > static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
Hello Thomas, On 5/11/22 13:47, Thomas Zimmermann wrote: > Hi Javier > > Am 11.05.22 um 13:30 schrieb Javier Martinez Canillas: >> Drivers that want to remove registered conflicting framebuffers prior to >> register their own framebuffer, calls remove_conflicting_framebuffers(). >> >> This function takes the registration_lock mutex, to prevent a races when >> drivers register framebuffer devices. But if a conflicting framebuffer >> device is found, the underlaying platform device is unregistered and this >> will lead to the platform driver .remove callback to be called, which in >> turn will call to the unregister_framebuffer() that takes the same lock. >> >> To prevent this, a struct fb_info.forced_out field was used as indication >> to unregister_framebuffer() whether the mutex has to be grabbed or not. >> >> A cleaner solution is to drop the lock before platform_device_unregister() >> so unregister_framebuffer() can take it when called from the fbdev driver, >> and just grab the lock again after the device has been registered and do >> a removal loop restart. >> >> Since the framebuffer devices will already be removed, the loop would just >> finish when no more conflicting framebuffers are found. >> >> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > I'd like to shrink this patchset. This looks like it can be merged Same. At least this version dropped a few patches that we had in v4 (related to DRM_FIRMWARE capability flag). > immediately? > Yes, this one is independent of the others and could be merged already. > Best regards > Thomas >
On 5/11/22 13:30, Javier Martinez Canillas wrote: > Drivers that want to remove registered conflicting framebuffers prior to > register their own framebuffer, calls remove_conflicting_framebuffers(). > > This function takes the registration_lock mutex, to prevent a races when > drivers register framebuffer devices. But if a conflicting framebuffer > device is found, the underlaying platform device is unregistered and this > will lead to the platform driver .remove callback to be called, which in > turn will call to the unregister_framebuffer() that takes the same lock. > > To prevent this, a struct fb_info.forced_out field was used as indication > to unregister_framebuffer() whether the mutex has to be grabbed or not. > > A cleaner solution is to drop the lock before platform_device_unregister() > so unregister_framebuffer() can take it when called from the fbdev driver, > and just grab the lock again after the device has been registered and do > a removal loop restart. > > Since the framebuffer devices will already be removed, the loop would just > finish when no more conflicting framebuffers are found. > > Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- Pushed this to drm-misc (drm-misc-next). Thanks all!
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index b445a7a00def..2fda5917c212 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -1555,6 +1555,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, { int i; +restart_removal: /* check all firmware fbs and kick off if the base addr overlaps */ for_each_registered_fb(i) { struct apertures_struct *gen_aper; @@ -1587,12 +1588,23 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a, pr_warn("fb%d: no device set\n", i); do_unregister_framebuffer(registered_fb[i]); } else if (dev_is_platform(device)) { - registered_fb[i]->forced_out = true; + /* + * Drop the lock because if the device is unregistered, its + * driver will call to unregister_framebuffer(), that takes + * this lock. + */ + mutex_unlock(®istration_lock); platform_device_unregister(to_platform_device(device)); + mutex_lock(®istration_lock); } else { pr_warn("fb%d: cannot remove device\n", i); do_unregister_framebuffer(registered_fb[i]); } + /* + * Restart the removal loop now that the device has been + * unregistered and its associated framebuffer gone. + */ + goto restart_removal; } } } @@ -1899,13 +1911,9 @@ EXPORT_SYMBOL(register_framebuffer); void unregister_framebuffer(struct fb_info *fb_info) { - bool forced_out = fb_info->forced_out; - - if (!forced_out) - mutex_lock(®istration_lock); + mutex_lock(®istration_lock); do_unregister_framebuffer(fb_info); - if (!forced_out) - mutex_unlock(®istration_lock); + mutex_unlock(®istration_lock); } EXPORT_SYMBOL(unregister_framebuffer); diff --git a/include/linux/fb.h b/include/linux/fb.h index 69c67c70fa78..bbe1e4571899 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -511,7 +511,6 @@ struct fb_info { } *apertures; bool skip_vt_switch; /* no VT switch on suspend/resume required */ - bool forced_out; /* set when being removed by another driver */ }; static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {