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

[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".

> 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);