diff mbox series

[2/4] fbdev/efifb: Use screen_info pointer from device

Message ID 20231129155218.3475-3-tzimmermann@suse.de
State New
Headers show
Series fbdev: Remove global screen_info in efifb/vesafb | expand

Commit Message

Thomas Zimmermann Nov. 29, 2023, 3:48 p.m. UTC
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(-)

Comments

Javier Martinez Canillas Dec. 1, 2023, 8:54 a.m. UTC | #1
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>
Thomas Zimmermann Dec. 1, 2023, 9:26 a.m. UTC | #2
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>
>
Javier Martinez Canillas Dec. 1, 2023, 9:56 a.m. UTC | #3
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 mbox series

Patch

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;