diff mbox series

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

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

Commit Message

Thomas Zimmermann Jan. 17, 2024, 12:39 p.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.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/firmware/sysfb.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

Javier Martinez Canillas Jan. 29, 2024, 11:36 a.m. UTC | #1
Thomas Zimmermann <tzimmermann@suse.de> writes:

> 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.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

>  
> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
> +{
> +#if defined(CONFIG_PCI)
> +	/*
> +	 * 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
> +	// Getting here without PCI support is probably a bug.
> +	return false;

Should we warn before return in this case ?

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Thomas Zimmermann Jan. 30, 2024, 12:52 p.m. UTC | #2
Hi

Am 29.01.24 um 12:36 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> 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.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>>   
>> +static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
>> +{
>> +#if defined(CONFIG_PCI)
>> +	/*
>> +	 * 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
>> +	// Getting here without PCI support is probably a bug.
>> +	return false;
> 
> Should we warn before return in this case ?

I would not do so as the bug is not here, but in screen_info_pci_dev(). 
I'm going to update this chunk such that the non-PCI case is a separate 
function.

> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
diff mbox series

Patch

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c
index 8a42da3f67a9..48a550bd1a93 100644
--- a/drivers/firmware/sysfb.c
+++ b/drivers/firmware/sysfb.c
@@ -70,6 +70,27 @@  void sysfb_disable(void)
 }
 EXPORT_SYMBOL_GPL(sysfb_disable);
 
+static __init bool sysfb_pci_dev_is_enabled(struct pci_dev *pdev)
+{
+#if defined(CONFIG_PCI)
+	/*
+	 * 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
+	// Getting here without PCI support is probably a bug.
+	return false;
+#endif
+}
+
 static __init int sysfb_init(void)
 {
 	const struct screen_info *si = &screen_info;
@@ -87,8 +108,11 @@  static __init int sysfb_init(void)
 	sysfb_apply_efi_quirks();
 
 	pparent = screen_info_pci_dev(si);
-	if (pparent)
+	if (pparent) {
+		if (!sysfb_pci_dev_is_enabled(pparent))
+			goto unlock_mutex;
 		parent = &pparent->dev;
+	}
 
 	/* try to create a simple-framebuffer device */
 	compatible = sysfb_parse_mode(si, &mode);