Re: [NOT A REGRESSION] firmware: framebuffer-coreboot: duplicate device name "simple-framebuffer.0"

[Javier Martinez Canillas <javierm@redhat.com>]

Brian Norris <briannorris@chromium.org> writes:

Hello Brian,

> Hi Javier,
>
> On Thu, Sep 12, 2024 at 06:33:58PM +0200, Javier Martinez Canillas wrote:
>> That's a very good point. I'm actually not familiar with Coreboot and I
>> used an educated guess (in the case of DT for example, that's the main
>> source of truth and I didn't know if a Core table was in a similar vein).
>> 
>> Maybe something like the following (untested) patch then?
>
> Julius is more familiar with the Coreboot + payload ecosystem than me,
> but his explanations make sense to me, as does this patch.
>
>> From de1c32017006f4671d91b695f4d6b4e99c073ab2 Mon Sep 17 00:00:00 2001
>> From: Javier Martinez Canillas <javierm@redhat.com>
>> Date: Thu, 12 Sep 2024 18:31:55 +0200
>> Subject: [PATCH] firmware: coreboot: Don't register a pdev if screen_info data
>>  is available
>> 
>> On Coreboot platforms, a system framebuffer may be provided to the Linux
>> kernel by filling a LB_TAG_FRAMEBUFFER entry in the Coreboot table. But
>> a Coreboot payload (e.g: SeaBIOS) could also provide this information to
>> the Linux kernel.
>> 
>> If that the case, early arch x86 boot code will fill the global struct
>> screen_info data and that data used by the Generic System Framebuffers
>> (sysfb) framework to add a platform device with platform data about the
>> system framebuffer.
>
> Normally, these sorts of "early" and "later" ordering descriptions would
> set alarm bells when talking about independent drivers. But I suppose
> the "early arch" code has better ordering guaranteeds than drivers, so
> this should be fine.
>

Yes, I didn't want to imply ordering here but just mentioning what code
was registering a "simple-framebuffer" platform_device, that conflicted
with this driver.

>> But later then the framebuffer_coreboot driver will try to do the same
>> framebuffer (using the information from the Coreboot table), which will
>> lead to an error due a simple-framebuffer.0 device already registered:
>> 

[...]

>>  
>> +	/*
>> +	 * If the global screen_info data has been filled, the Generic
>> +	 * System Framebuffers (sysfb) will already register a platform
>
> Did you mean 'platform_device'?
>

Ups, yeah I forgot to write device there.

>> +	 * and pass the screen_info as platform_data to a driver that
>> +	 * could scan-out using the system provided framebuffer.
>> +	 *
>> +	 * On Coreboot systems, the advertise LB_TAG_FRAMEBUFFER entry
>
> s/advertise/advertised/ ?
>

Ok.

        >> +	 * in the Coreboot table should only be used if the payload did
>> +	 * not set video mode info and passed it to the Linux kernel.
>
> s/passed/pass/
>

Ok.

>> +	 */
>> +	if (si->orig_video_isVGA == VIDEO_TYPE_VLFB ||
>> +            si->orig_video_isVGA == VIDEO_TYPE_EFI)
>
> This line is using spaces for indentation. It should use a tab, and then
> spaces for alignment. But presumably this will change based on Thomas's
> suggestions anyway.
>

Yes, I usually run checkpatch --strict before posting but didn't in this
case because just shared the patch as a response.

>> +		return -EINVAL;
>
> Is EINVAL right? IIUC, that will print a noisier error to the logs. I
> believe the "expected" sorts of return codes are ENODEV or ENXIO. (See
> call_driver_probe().) ENODEV seems like a fine choice, similar to
> several of the other return codes already used here.
>

You are right, -ENODEV is indeed a more suitable error code for this.

> Anyway, this seems along the right track. Thanks for tackling, and feel
> free to carry a:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
>

Thanks and for your comments.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat