diff mbox series

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

Message ID 20240202120140.3517-6-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series firmware/sysfb: Track parent device for screen_info | expand

Commit Message

Thomas Zimmermann Feb. 2, 2024, 11:58 a.m. UTC
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.

v2:
	* rework sysfb_pci_dev_is_enabled() (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

Comments

Sui Jingfeng Feb. 2, 2024, 5:50 p.m. UTC | #1
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".

> v2:
> 	* rework sysfb_pci_dev_is_enabled() (Javier)
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@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 Feb. 5, 2024, 8:25 a.m. UTC | #2
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@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@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);
diff mbox series

Patch

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);
 		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;
 
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);