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

[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
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat