Hi Am 05.02.24 um 11:05 schrieb Sui Jingfeng:
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 the3/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 onthe 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 followingproblem:
Right, that's part of the bug you already pointed out. As I said, I need to review the callers of screen_info_pci_dev() and fix them.
But returning an errno pointer is useful in cases where a possible parent device would have been detected, but something did not work out. For example, screen_info_resources() does not support all VIDEO_TYPE_ constants. In such a case, it's better to tell the caller about the problem than to silently return NULL.
Best regards Thomas
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
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
