From: Sui Jingfeng <sui.jingfeng@linux.dev>
To: Thomas Zimmermann <tzimmermann@suse.de>,
javierm@redhat.com, pjones@redhat.com, deller@gmx.de,
ardb@kernel.org
Cc: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org
Subject: Re: [v2,2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device
Date: Mon, 5 Feb 2024 18:05:02 +0800 [thread overview]
Message-ID: <1d28e615-4c40-4c43-ac97-915ca25e1cd7@linux.dev> (raw)
In-Reply-To: <0293822c-b261-4725-8cca-3b6dd8e2991d@suse.de>
Hi,
On 2024/2/5 16:17, Thomas Zimmermann wrote:
> Hi
>
> Am 02.02.24 um 18:03 schrieb Sui Jingfeng:
>> Hi,
>>
>>
>> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>>> +
>>> +/**
>>> + * screen_info_pci_dev() - Return PCI parent device that contains
>>> screen_info's framebuffer
>>> + * @si: the screen_info
>>> + *
>>> + * Returns:
>>> + * The screen_info's parent device on success, or NULL otherwise.
>>> + */
>>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>>> +{
>>> + struct resource res[SCREEN_INFO_MAX_RESOURCES];
>>> + ssize_t i, numres;
>>> +
>>> + numres = screen_info_resources(si, res, ARRAY_SIZE(res));
>>> + if (numres < 0)
>>> + return ERR_PTR(numres);
>>
>>
>> Please return NULL at here, otherwise we have to use the IS_ERR or
>> IS_ERR_OR_NULL()
>> in the caller function to check the returned value. Meanwhile, I
>> noticed that you
>> didn't actually call IS_ERR() in the sysfb_parent_dev() function
>> (introduced by the
>> 3/8 patch), so I think we probably should return NULL at here.
>>
>> Please also consider that the comments of this function says that it
>> return NULL on
>> the otherwise cases.
>
> Right. The idea is to return NULL is there is no parent device.
return NULL is more easier and clear, it stand for "None" or "don't exist".
There is another reason that I want to tell you:
Some systems which don't have a good UEFI firmware support for uncommon GPUs.
the word "uncommon" means "not very popular GPU" or "extremely new GPU" or
"just refer to the GPUs that UEFI firmware don't know(recognize) about"
On such cases, there is no firmware framebuffer support. I means it is possible
that screen_info_resources() return -EINVAL because of not support yet. Then,
the screen_info_pci_dev(si) returns ERR_PTR(-EINVAL) and sysfb_pci_dev_is_enabled()
will take this error code as a pointer and de-reference it, cause the following
problem:
And even the x86-64 motherboard will not likely support all GPU(for example the one
with a old UEFI BIOS). And for an example, The intel Xe is the "extremely new GPU".
[ 5.031966] CPU 4 Unable to handle kernel paging request at virtual
address 000000000000081a, era == 900000000329b448, ra == 900000000329b440
[ 5.044587] Oops[#1]:
[ 5.046837] CPU: 4 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc1+ #7
[ 5.053062] Hardware name: Loongson
Loongson-3A6000-HV-7A2000-XA61200/Loongson-3A6000-HV-7A2000-XA61200,
BIOS Loongson-UDK2018-V4.0.05636-stable202311 12/
[ 5.066803] pc 900000000329b448 ra 900000000329b440 tp
90000001000d0000 sp 90000001000d3d40
[ 5.075100] a0 ffffffffffffffea a1 90000001000d3c38 a2
0000000000000003 a3 9000000003867ce8
[ 5.083398] a4 9000000003867ce0 a5 90000001000d3a80 a6
0000000000000001 a7 0000000000000001
[ 5.091695] t0 ac81f55e34713962 t1 ac81f55e34713962 t2
0000000000000000 t3 0000000000000001
[ 5.099992] t4 0000000000000004 t5 0000000000000000 t6
0000000000000030 t7 0000000000000000
[ 5.108290] t8 00000000000070b1 u0 0000000000000000 s9
0000000000000008 s0 9000000003d58b48
[ 5.116587] s1 9000000003c0b4a8 s2 9000000003787000 s3
9000000003778000 s4 90000000032c0578
[ 5.124884] s5 ffffffffffffffea s6 90000000032c0560 s7
90000000032df900 s8 ffffffffccccc000
[ 5.133182] ra: 900000000329b440 sysfb_init+0x80/0x1f0
[ 5.138545] ERA: 900000000329b448 sysfb_init+0x88/0x1f0
[ 5.143905] CRMD: 000000b0 (PLV0 -IE -DA +PG DACF=CC DACM=CC -WE)
[ 5.150048] PRMD: 00000004 (PPLV0 +PIE -PWE)
[ 5.154373] EUEN: 00000000 (-FPE -SXE -ASXE -BTE)
[ 5.159131] ECFG: 00071c1c (LIE=2-4,10-12 VS=7)
[ 5.163717] ESTAT: 00010000 [PIL] (IS= ECode=1 EsubCode=0)
[ 5.169164] BADV: 000000000000081a
[ 5.172623] PRID: 0014d000 (Loongson-64bit, Loongson-3A6000-HV)
[ 5.178587] Modules linked in:
[ 5.181614] Process swapper/0 (pid: 1, threadinfo=(____ptrval____),
task=(____ptrval____))
[ 5.189827] Stack : 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[ 5.197782] 0000000000000000 ac81f55e34713962
90000000032c0000 9000000003778000
[ 5.205736] 90000000032c0578 0000000000000000
900000000329b3c0 9000000003c54000
[ 5.213691] 90000001000d3db8 9000000002260154
0000000000000006 0000000000000000
[ 5.221645] 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[ 5.229599] 0000000000000000 0000000000000000
0000000000000000 ac81f55e34713962
[ 5.237553] 90000000037468f8 90000000037468f8
90000000032c0578 90000000036a7658
[ 5.245508] 0000000000000006 9000000100041e00
0000000000000a55 9000000003260ff4
[ 5.251549] ata3: SATA link down (SStatus 0 SControl 300)
[ 5.253463] 0000000000000000 90000000032600b0
0000000000000000 0000000000000000
[ 5.266777] 0000000000000000 0000000000000000
0000000000000000 0000000000000000
[ 5.274731] ...
[ 5.277154] Call Trace:
[ 5.277155] [<900000000329b448>] sysfb_init+0x88/0x1f0
[ 5.284678] [<9000000002260154>] do_one_initcall+0x78/0x1cc
[ 5.290213] [<9000000003260ff4>] kernel_init_freeable+0x228/0x298
[ 5.296267] [<900000000324d104>] kernel_init+0x20/0x110
[ 5.301455] [<90000000022611e8>] ret_from_kernel_thread+0xc/0xa4
[ 5.307421]
[ 5.308892] Code: 561667fe 0015009c 40007080 <2408308c> 29403860
02c2e09b 0040818c 6400180c 1a007d45
[ 5.318579]
[ 5.320053] ---[ end trace 0000000000000000 ]---
[ 5.324640] Kernel panic - not syncing: Attempted to kill init!
exitcode=0x0000000b
[ 5.332247] Kernel relocated by 0x2040000
[ 5.336226] .text @ 0x9000000002240000
[ 5.340031] .data @ 0x90000000032f0000
[ 5.343835] .bss @ 0x9000000003c3f200
[ 5.347640] ---[ end Kernel panic - not syncing: Attempted to kill
init! exitcode=0x0000000b ]---
> Best regards
> Thomas
>
next prev parent reply other threads:[~2024-02-05 10:05 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-02 11:58 [PATCH v2 0/8] firmware/sysfb: Track parent device for screen_info Thomas Zimmermann
2024-02-02 11:58 ` [PATCH v2 1/8] video: Add helpers for decoding screen_info Thomas Zimmermann
2024-02-02 11:58 ` [PATCH v2 2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device Thomas Zimmermann
2024-02-02 16:31 ` [v2,2/8] " Sui Jingfeng
2024-02-05 8:14 ` Thomas Zimmermann
2024-02-02 17:03 ` Sui Jingfeng
2024-02-05 8:17 ` Thomas Zimmermann
2024-02-05 10:05 ` Sui Jingfeng [this message]
2024-02-05 12:32 ` Thomas Zimmermann
2024-02-04 1:53 ` [PATCH v2 2/8] " kernel test robot
2024-02-02 11:58 ` [PATCH v2 3/8] firmware/sysfb: Set firmware-framebuffer parent device Thomas Zimmermann
2024-02-02 14:40 ` [v2,3/8] " Sui Jingfeng
2024-02-02 15:23 ` Sui Jingfeng
2024-02-05 8:24 ` Thomas Zimmermann
2024-02-07 15:34 ` Sui Jingfeng
2024-02-03 19:12 ` [PATCH v2 3/8] " kernel test robot
2024-02-04 2:13 ` kernel test robot
2024-02-02 11:58 ` [PATCH v2 4/8] fbdev/efifb: Remove PM for " Thomas Zimmermann
2024-02-02 11:58 ` [PATCH v2 5/8] firmware/sysfb: Create firmware device only for enabled PCI devices Thomas Zimmermann
2024-02-02 17:50 ` [v2,5/8] " Sui Jingfeng
2024-02-05 8:25 ` [v2, 5/8] " Thomas Zimmermann
2024-02-02 11:58 ` [PATCH v2 6/8] fbdev/efifb: Do not track parent device status Thomas Zimmermann
2024-02-02 11:58 ` [PATCH v2 7/8] firmware/sysfb: Update screen_info for relocated EFI framebuffers Thomas Zimmermann
2024-02-02 17:54 ` [v2,7/8] " Sui Jingfeng
2024-02-05 10:11 ` Thomas Zimmermann
2024-02-02 18:00 ` Sui Jingfeng
2024-02-06 16:45 ` Thomas Zimmermann
2024-02-02 11:58 ` [PATCH v2 8/8] fbdev/efifb: Remove framebuffer relocation tracking Thomas Zimmermann
2024-02-02 18:07 ` [v2,8/8] " Sui Jingfeng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1d28e615-4c40-4c43-ac97-915ca25e1cd7@linux.dev \
--to=sui.jingfeng@linux.dev \
--cc=ardb@kernel.org \
--cc=deller@gmx.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=javierm@redhat.com \
--cc=linux-fbdev@vger.kernel.org \
--cc=pjones@redhat.com \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).