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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

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@xxxxxxx>
> > Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> > ---
> >   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)

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature