[v2, 5/8] firmware/sysfb: Create firmware device only for enabled PCI devices

[Thomas Zimmermann]

Am 02.02.24 um 18:50 schrieb Sui Jingfeng:
> HI,
> 
> 
> On 2024/2/2 19:58, Thomas Zimmermann wrote:
>> Test if the firmware framebuffer's parent PCI device, if any, has
>> been enabled. If not, the firmware framebuffer is most likely not
>> working. Hence, do not create a device for the firmware framebuffer
>> on disabled PCI devices.
>>
>> So far, efifb tracked the status of the PCI parent device internally
>> and did not bind if it was disabled. This patch implements the
>> functionality for all firmware framebuffers.
> 
> 
> For *all* ?
> 
> I think the functionality this patch implemented is only target for the
> PCIe device firmware framebuffers, the framebuffer consumed by the simplefb
> driver (fbdev/simplefb.c) is also a kind of firmware framebuffer, but it is
> target for the platform device only.
> 
> So, the correct description would be: "this patch implements the 
> functionality
> for the PCIe firmware framebuffers".

Fair enough.

> 
>> v2:
>>     * rework sysfb_pci_dev_is_enabled() (Javier)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
>> ---
>>   drivers/firmware/sysfb.c | 30 +++++++++++++++++++++++++++++-
>>   1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
>> index d02945b0d8ea1..ab5cbc0326f6d 100644
>> --- a/drivers/firmware/sysfb.c
>> +++ b/drivers/firmware/sysfb.c
>> @@ -70,13 +70,39 @@ void sysfb_disable(void)
>>   }
>>   EXPORT_SYMBOL_GPL(sysfb_disable);
>> +#if defined(CONFIG_PCI)
>> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
>> +{
>> +    /*
>> +     * TODO: Try to integrate this code into the PCI subsystem
>> +     */
>> +    int ret;
>> +    u16 command;
>> +
>> +    ret = pci_read_config_word(pdev, PCI_COMMAND, &command);
>> +    if (ret != PCIBIOS_SUCCESSFUL)
>> +        return false;
>> +    if (!(command & PCI_COMMAND_MEMORY))
>> +        return false;
>> +    return true;
>> +}
>> +#else
>> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
>> +{
>> +    return false;
>> +}
>> +#endif
>> +
>>   static __init struct device *sysfb_parent_dev(const struct 
>> screen_info *si)
>>   {
>>       struct pci_dev *pdev;
>>       pdev = screen_info_pci_dev(si);
>> -    if (pdev)
>> +    if (pdev) {
>> +        if (!sysfb_pci_dev_is_enabled(pdev))
>> +            return ERR_PTR(-ENODEV);
> 
> 
> Is it better to move the call of sysfb_pci_dev_is_enabled() out of 
> sysfb_parent_dev() ?
> Because then we don't need check the returned value by calling the 
> IS_ERR() inthe sysfb_init() function.
> 
> 
>>           return &pdev->dev;
>> +    }
>>       return NULL;
>>   }
>> @@ -97,6 +123,8 @@ static __init int sysfb_init(void)
>>       sysfb_apply_efi_quirks();
>>       parent = sysfb_parent_dev(si);
>> +    if (IS_ERR(parent))
>> +        goto unlock_mutex;
> 
>      if (!sysfb_pci_dev_is_enabled(parent))
>          goto unlock_mutex;
> 
>>       /* try to create a simple-framebuffer device */
>>       compatible = sysfb_parse_mode(si, &mode);

-- 
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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240205/5e0c26ec/attachment.sig>