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