| Message ID | 20231129155218.3475-3-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | fbdev: Remove global screen_info in efifb/vesafb | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: > Use the screen_info instance from the device instead of dereferencing > the global screen_info state. Decouples the driver from per-architecture > code. Duplicated the screen_info data, so that efifb can modify it at > will. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- [...] > + si = dev_get_platdata(&dev->dev); > + if (!si) I would add a comment that this platform data is set when the device is registered by sysfb. > + return -ENODEV; > + si = devm_kmemdup(&dev->dev, si, sizeof(*si), GFP_KERNEL); > + if (!si) > + return -ENOMEM; > + Why a copy? In any case maybe the global screen_info should be duplicated when is set as the device platform data in sysfb_init() ? I agree with the direction of the patch though, so whatever you decide: Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Javier Am 01.12.23 um 09:54 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > >> Use the screen_info instance from the device instead of dereferencing >> the global screen_info state. Decouples the driver from per-architecture >> code. Duplicated the screen_info data, so that efifb can modify it at >> will. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- > > [...] > >> + si = dev_get_platdata(&dev->dev); >> + if (!si) > > I would add a comment that this platform data is set when the device is > registered by sysfb. > >> + return -ENODEV; >> + si = devm_kmemdup(&dev->dev, si, sizeof(*si), GFP_KERNEL); >> + if (!si) >> + return -ENOMEM; >> + > > Why a copy? In any case maybe the global screen_info should be duplicated > when is set as the device platform data in sysfb_init() ? We get our own copy of the global screen_info as platform-device data. Efifb modifies some of the values in our copy in efifb_setup(). If probing afterwards fails, the kernel might try a different driver, which would then operate on the values modified by efifb. Hence, there's this internal copy. The situation with vesafb is similar. Best regards Thomas > > I agree with the direction of the patch though, so whatever you decide: > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi Javier > > Am 01.12.23 um 09:54 schrieb Javier Martinez Canillas: >> Thomas Zimmermann <tzimmermann@suse.de> writes: >> >>> Use the screen_info instance from the device instead of dereferencing >>> the global screen_info state. Decouples the driver from per-architecture >>> code. Duplicated the screen_info data, so that efifb can modify it at >>> will. >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >> >> [...] >> >>> + si = dev_get_platdata(&dev->dev); >>> + if (!si) >> >> I would add a comment that this platform data is set when the device is >> registered by sysfb. >> >>> + return -ENODEV; >>> + si = devm_kmemdup(&dev->dev, si, sizeof(*si), GFP_KERNEL); >>> + if (!si) >>> + return -ENOMEM; >>> + >> >> Why a copy? In any case maybe the global screen_info should be duplicated >> when is set as the device platform data in sysfb_init() ? > > We get our own copy of the global screen_info as platform-device data. Ah, I didn't notice that platform_device_add_data() already did a kmemdup(). > Efifb modifies some of the values in our copy in efifb_setup(). If > probing afterwards fails, the kernel might try a different driver, which > would then operate on the values modified by efifb. Hence, there's this > internal copy. The situation with vesafb is similar. > I see what you mean. I was thinking that the same device coulnd't be match to a different driver anyways but it's true that a fail to would make that possible. > Best regards > Thomas >
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index 6cbb65bbe1110..d0ce357ff2684 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -358,7 +358,7 @@ static u64 bar_offset; static int efifb_probe(struct platform_device *dev) { - struct screen_info *si = &screen_info; + struct screen_info *si; struct fb_info *info; struct efifb_par *par; int err, orientation; @@ -368,6 +368,13 @@ static int efifb_probe(struct platform_device *dev) char *option = NULL; efi_memory_desc_t md; + si = dev_get_platdata(&dev->dev); + if (!si) + return -ENODEV; + si = devm_kmemdup(&dev->dev, si, sizeof(*si), GFP_KERNEL); + if (!si) + return -ENOMEM; + if (si->orig_video_isVGA != VIDEO_TYPE_EFI || pci_dev_disabled) return -ENODEV;
Use the screen_info instance from the device instead of dereferencing the global screen_info state. Decouples the driver from per-architecture code. Duplicated the screen_info data, so that efifb can modify it at will. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/video/fbdev/efifb.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)