Re: [PATCH 3/5] fbdev: Introduce devm_register_framebuffer()

[Helge Deller]

On 8/30/24 10:25, Bert Karwatzki wrote:
> Am Freitag, dem 30.08.2024 um 09:17 +0200 schrieb Thomas Weißschuh:
> > Hi everybody,
> >
> > On 2024-08-27 17:25:14+0000, Thomas Weißschuh wrote:
> > > Introduce a device-managed variant of register_framebuffer() which
> > > automatically unregisters the framebuffer on device destruction.
> > > This can simplify the error handling and resource management in drivers.
> >
> > Bert reported that this series broke his framebuffer ([0], [1]).
> >
> > [0] https://lore.kernel.org/lkml/20240829224124.2978-1-spasswolf@xxxxxx/
> > [1] https://lore.kernel.org/lkml/20240829230438.3226-1-spasswolf@xxxxxx/
> >
> > > Signed-off-by: Thomas Weißschuh <linux@xxxxxxxxxxxxxx>
> > > ---
> > >   drivers/video/fbdev/core/fbmem.c | 24 ++++++++++++++++++++++++
> > >   include/linux/fb.h               |  1 +
> > >   2 files changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > > index 4c4ad0a86a50..d17a2daa2483 100644
> > > --- a/drivers/video/fbdev/core/fbmem.c
> > > +++ b/drivers/video/fbdev/core/fbmem.c
> > > @@ -544,6 +544,30 @@ unregister_framebuffer(struct fb_info *fb_info)
> >
> > [..]
> >
> > > +/**
> > > + *	devm_register_framebuffer - resource-managed frame buffer device registration
> > > + *	@dev: device the framebuffer belongs to
> > > + *	@fb_info: frame buffer info structure
> > > + *
> > > + *	Registers a frame buffer device @fb_info to device @dev.
> > > + *
> > > + *	Returns negative errno on error, or zero for success.
> > > + *
> > > + */
> > > +int
> > > +devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
> > > +{
> > > +	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
> > > +}
> > > +EXPORT_SYMBOL(devm_register_framebuffer);
> >
> > This implementation is wrong, it never actually registers the
> > framebuffer. It should look like this:
> >
> > int
> > devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
> > {
> > 	int ret;
> >
> > 	ret = register_framebuffer(fb_info);
> > 	if (ret)
> > 		return ret;
> >
> > 	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
> > }
> > EXPORT_SYMBOL(devm_register_framebuffer);
> >
> > Bert, could you test this?
> > Helge, do you want me to resend the series, minus the original patch 1?
>
> Yes, this works for me. Thanks!

Good.

Thomas, please just resend the fixed patch #3 (this one).
And maybe you want to document devm_unregister_framebuffer()
similiar to what you added for devm_register_framebuffer() ?

Helge