| Message ID | 20220406213919.600294-2-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Fix some race conditions that exists between fbmem and sysfb | expand |
On Wed, Apr 06, 2022 at 11:39:15PM +0200, Javier Martinez Canillas wrote: > This function just returned 0 on success or an errno code on error, but it > could be useful to sysfb_init() to get a pointer to the device registered. > > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> You need to rebase this onto 202c08914ba5 ("firmware: sysfb: fix platform-device leak in error path") which fixes the same error path leak you are fixing in here too. Or we just have a neat conflict when merging :-) But in that case please mention that you fix the error path leak too so it's less confusing when Linus or someone needs to resolve the conflict. Anyway Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > > drivers/firmware/sysfb.c | 4 ++-- > drivers/firmware/sysfb_simplefb.c | 24 +++++++++++++++--------- > include/linux/sysfb.h | 10 +++++----- > 3 files changed, 22 insertions(+), 16 deletions(-) > > diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c > index 2bfbb05f7d89..b032f40a92de 100644 > --- a/drivers/firmware/sysfb.c > +++ b/drivers/firmware/sysfb.c > @@ -46,8 +46,8 @@ static __init int sysfb_init(void) > /* try to create a simple-framebuffer device */ > compatible = sysfb_parse_mode(si, &mode); > if (compatible) { > - ret = sysfb_create_simplefb(si, &mode); > - if (!ret) > + pd = sysfb_create_simplefb(si, &mode); > + if (!IS_ERR(pd)) > return 0; > } > > diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c > index 76c4abc42a30..c42648ed3aad 100644 > --- a/drivers/firmware/sysfb_simplefb.c > +++ b/drivers/firmware/sysfb_simplefb.c > @@ -57,8 +57,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si, > return false; > } > > -__init int sysfb_create_simplefb(const struct screen_info *si, > - const struct simplefb_platform_data *mode) > +__init struct platform_device *sysfb_create_simplefb(const struct screen_info *si, > + const struct simplefb_platform_data *mode) > { > struct platform_device *pd; > struct resource res; > @@ -76,7 +76,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si, > base |= (u64)si->ext_lfb_base << 32; > if (!base || (u64)(resource_size_t)base != base) { > printk(KERN_DEBUG "sysfb: inaccessible VRAM base\n"); > - return -EINVAL; > + return ERR_PTR(-EINVAL); > } > > /* > @@ -93,7 +93,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si, > length = mode->height * mode->stride; > if (length > size) { > printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n"); > - return -EINVAL; > + return ERR_PTR(-EINVAL); > } > length = PAGE_ALIGN(length); > > @@ -104,25 +104,31 @@ __init int sysfb_create_simplefb(const struct screen_info *si, > res.start = base; > res.end = res.start + length - 1; > if (res.end <= res.start) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > pd = platform_device_alloc("simple-framebuffer", 0); > if (!pd) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > sysfb_apply_efi_quirks(pd); > > ret = platform_device_add_resources(pd, &res, 1); > if (ret) { > platform_device_put(pd); > - return ret; > + return ERR_PTR(ret); > } > > ret = platform_device_add_data(pd, mode, sizeof(*mode)); > if (ret) { > platform_device_put(pd); > - return ret; > + return ERR_PTR(ret); > } > > - return platform_device_add(pd); > + ret = platform_device_add(pd); > + if (ret) { > + platform_device_put(pd); > + return ERR_PTR(ret); > + } > + > + return pd; > } > diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h > index b0dcfa26d07b..708152e9037b 100644 > --- a/include/linux/sysfb.h > +++ b/include/linux/sysfb.h > @@ -72,8 +72,8 @@ static inline void sysfb_apply_efi_quirks(struct platform_device *pd) > > bool sysfb_parse_mode(const struct screen_info *si, > struct simplefb_platform_data *mode); > -int sysfb_create_simplefb(const struct screen_info *si, > - const struct simplefb_platform_data *mode); > +struct platform_device *sysfb_create_simplefb(const struct screen_info *si, > + const struct simplefb_platform_data *mode); > > #else /* CONFIG_SYSFB_SIMPLE */ > > @@ -83,10 +83,10 @@ static inline bool sysfb_parse_mode(const struct screen_info *si, > return false; > } > > -static inline int sysfb_create_simplefb(const struct screen_info *si, > - const struct simplefb_platform_data *mode) > +static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si, > + const struct simplefb_platform_data *mode) > { > - return -EINVAL; > + return ERR_PTR(-EINVAL); > } > > #endif /* CONFIG_SYSFB_SIMPLE */ > -- > 2.35.1 >
Hello Daniel, On 4/7/22 11:03, Daniel Vetter wrote: > On Wed, Apr 06, 2022 at 11:39:15PM +0200, Javier Martinez Canillas wrote: >> This function just returned 0 on success or an errno code on error, but it >> could be useful to sysfb_init() to get a pointer to the device registered. >> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> > > You need to rebase this onto 202c08914ba5 ("firmware: sysfb: fix > platform-device leak in error path") which fixes the same error path leak > you are fixing in here too. Or we just have a neat conflict when merging > :-) But in that case please mention that you fix the error path leak too > so it's less confusing when Linus or someone needs to resolve the > conflict. > Ups, I thought that had my local tree up-to-date but it seems that was a few days old. I've updated my remote now and rebased, so will have this fixed in the next revision of the series. And this patch becomes smaller indeed :) > Anyway Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Thanks!
diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index 2bfbb05f7d89..b032f40a92de 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -46,8 +46,8 @@ static __init int sysfb_init(void) /* try to create a simple-framebuffer device */ compatible = sysfb_parse_mode(si, &mode); if (compatible) { - ret = sysfb_create_simplefb(si, &mode); - if (!ret) + pd = sysfb_create_simplefb(si, &mode); + if (!IS_ERR(pd)) return 0; } diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c index 76c4abc42a30..c42648ed3aad 100644 --- a/drivers/firmware/sysfb_simplefb.c +++ b/drivers/firmware/sysfb_simplefb.c @@ -57,8 +57,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si, return false; } -__init int sysfb_create_simplefb(const struct screen_info *si, - const struct simplefb_platform_data *mode) +__init struct platform_device *sysfb_create_simplefb(const struct screen_info *si, + const struct simplefb_platform_data *mode) { struct platform_device *pd; struct resource res; @@ -76,7 +76,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si, base |= (u64)si->ext_lfb_base << 32; if (!base || (u64)(resource_size_t)base != base) { printk(KERN_DEBUG "sysfb: inaccessible VRAM base\n"); - return -EINVAL; + return ERR_PTR(-EINVAL); } /* @@ -93,7 +93,7 @@ __init int sysfb_create_simplefb(const struct screen_info *si, length = mode->height * mode->stride; if (length > size) { printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n"); - return -EINVAL; + return ERR_PTR(-EINVAL); } length = PAGE_ALIGN(length); @@ -104,25 +104,31 @@ __init int sysfb_create_simplefb(const struct screen_info *si, res.start = base; res.end = res.start + length - 1; if (res.end <= res.start) - return -EINVAL; + return ERR_PTR(-EINVAL); pd = platform_device_alloc("simple-framebuffer", 0); if (!pd) - return -ENOMEM; + return ERR_PTR(-ENOMEM); sysfb_apply_efi_quirks(pd); ret = platform_device_add_resources(pd, &res, 1); if (ret) { platform_device_put(pd); - return ret; + return ERR_PTR(ret); } ret = platform_device_add_data(pd, mode, sizeof(*mode)); if (ret) { platform_device_put(pd); - return ret; + return ERR_PTR(ret); } - return platform_device_add(pd); + ret = platform_device_add(pd); + if (ret) { + platform_device_put(pd); + return ERR_PTR(ret); + } + + return pd; } diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h index b0dcfa26d07b..708152e9037b 100644 --- a/include/linux/sysfb.h +++ b/include/linux/sysfb.h @@ -72,8 +72,8 @@ static inline void sysfb_apply_efi_quirks(struct platform_device *pd) bool sysfb_parse_mode(const struct screen_info *si, struct simplefb_platform_data *mode); -int sysfb_create_simplefb(const struct screen_info *si, - const struct simplefb_platform_data *mode); +struct platform_device *sysfb_create_simplefb(const struct screen_info *si, + const struct simplefb_platform_data *mode); #else /* CONFIG_SYSFB_SIMPLE */ @@ -83,10 +83,10 @@ static inline bool sysfb_parse_mode(const struct screen_info *si, return false; } -static inline int sysfb_create_simplefb(const struct screen_info *si, - const struct simplefb_platform_data *mode) +static inline struct platform_device *sysfb_create_simplefb(const struct screen_info *si, + const struct simplefb_platform_data *mode) { - return -EINVAL; + return ERR_PTR(-EINVAL); } #endif /* CONFIG_SYSFB_SIMPLE */
This function just returned 0 on success or an errno code on error, but it could be useful to sysfb_init() to get a pointer to the device registered. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/firmware/sysfb.c | 4 ++-- drivers/firmware/sysfb_simplefb.c | 24 +++++++++++++++--------- include/linux/sysfb.h | 10 +++++----- 3 files changed, 22 insertions(+), 16 deletions(-)