Re: [PATCH v3 2/5] firmware: sysfb: Add helpers to unregister a pdev and disable registration

Hi

Am 20.04.22 um 10:53 schrieb Javier Martinez Canillas:

These can be used by subsystems to unregister a platform device registered by sysfb and also to disable future platform device registration in sysfb.

Suggested-by: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Javier Martinez Canillas javierm@redhat.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

(no changes since v2)

Changes in v2:

• Add kernel-doc comments and include in other_interfaces.rst (Daniel Vetter).

  .../driver-api/firmware/other_interfaces.rst | 6 ++ drivers/firmware/sysfb.c | 73 +++++++++++++++++-- include/linux/sysfb.h | 19 +++++ 3 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/firmware/other_interfaces.rst b/Documentation/driver-api/firmware/other_interfaces.rst index b81794e0cfbb..06ac89adaafb 100644 --- a/Documentation/driver-api/firmware/other_interfaces.rst +++ b/Documentation/driver-api/firmware/other_interfaces.rst @@ -13,6 +13,12 @@ EDD Interfaces .. kernel-doc:: drivers/firmware/edd.c :internal:

+Generic System Framebuffers Interface +-------------------------------------

+.. kernel-doc:: drivers/firmware/sysfb.c

• :export:

• Intel Stratix10 SoC Service Layer

  Some features of the Intel Stratix10 SoC require a level of privilege

diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index b032f40a92de..a50d2858ce4d 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -34,21 +34,78 @@ #include <linux/screen_info.h> #include <linux/sysfb.h>

+static struct platform_device *pd;

You could have multiple instances of simple-framebuffer because you don't know what the firmware set up. It originated from DT and EFI/VESA is really just an afterthought. If there is more than 1 instance, your entire unregistering breaks down.

(I've been told that such multi-output cases exist in the context of OF framebuffers. Something similar could exist for simple-frambuffer as well.)

Best regards Thomas

+static DEFINE_MUTEX(load_lock); +static bool disabled;

+/**

• • sysfb_disable() - disable the Generic System Framebuffers support
• • This disables the registration of system framebuffer devices that match the
• • generic drivers that make use of the system framebuffer set up by firmware.
• • Context: The function can sleep. A @load_lock mutex is acquired to serialize

• •      against sysfb_init(), that registers a system framebuffer device and
    

• •      sysfb_try_unregister(), that tries to unregister framebuffer devices.
    

• */

+void sysfb_disable(void) +{

• mutex_lock(&load_lock);
• disabled = true;
• mutex_unlock(&load_lock);

+} +EXPORT_SYMBOL_GPL(sysfb_disable);

+/**

• • sysfb_try_unregister() - attempt to unregister a system framebuffer device
• • @dev: device to unregister
• • This tries to unregister a system framebuffer device if this was registered
• • by the Generic System Framebuffers. The device will only be unregistered if
• • it was registered by sysfb_init(), otherwise it will not be unregistered.
• • Context: The function can sleep. a @load_lock mutex is acquired to serialize

• •      against sysfb_init(), that registers a simple framebuffer device and
    

• •      sysfb_disable(), that disables the Generic System Framebuffers support.
    

• • Return:
• • • true - the device was unregistered successfully
• • • false - the device was not unregistered
• */

+bool sysfb_try_unregister(struct device *dev) +{

• bool ret = true;
• mutex_lock(&load_lock);
• if (!pd || pd != to_platform_device(dev))

• return false;
  

• platform_device_unregister(to_platform_device(dev));
• pd = NULL;
• mutex_unlock(&load_lock);
• return ret;

+} +EXPORT_SYMBOL_GPL(sysfb_try_unregister);

• static __init int sysfb_init(void) { struct screen_info *si = &screen_info; struct simplefb_platform_data mode;

• struct platform_device *pd; const char *name; bool compatible;
• int ret;

• int ret = 0;

• mutex_lock(&load_lock);

• if (disabled)

• goto unlock_mutex;
  
  

  /* try to create a simple-framebuffer device */ compatible = sysfb_parse_mode(si, &mode); if (compatible) { pd = sysfb_create_simplefb(si, &mode); if (!IS_ERR(pd))

• 	return 0;
  

• 	goto unlock_mutex;
  

  }

  /* if the FB is incompatible, create a legacy framebuffer device */

@@ -60,8 +117,10 @@ static __init int sysfb_init(void) name = "platform-framebuffer";

pd = platform_device_alloc(name, 0);

• if (!pd)

• return -ENOMEM;
  

• if (!pd) {

• ret = -ENOMEM;
  

• goto unlock_mutex;
  

• }

  sysfb_apply_efi_quirks(pd);

@@ -73,9 +132,11 @@ static __init int sysfb_init(void) if (ret) goto err;

• return 0;

• goto unlock_mutex; err: platform_device_put(pd);

+unlock_mutex:

• mutex_unlock(&load_lock); return ret; }

diff --git a/include/linux/sysfb.h b/include/linux/sysfb.h index 708152e9037b..e8c0313fac8f 100644 --- a/include/linux/sysfb.h +++ b/include/linux/sysfb.h @@ -55,6 +55,25 @@ struct efifb_dmi_info { int flags; };

+#ifdef CONFIG_SYSFB

+void sysfb_disable(void); +bool sysfb_try_unregister(struct device *dev);

+#else /* CONFIG_SYSFB */

+static inline void sysfb_disable(void) +{

+}

+static inline bool sysfb_try_unregister(struct device *dev) +{

• return false;

+}

+#endif /* CONFIG_SYSFB */

• #ifdef CONFIG_EFI

  extern struct efifb_dmi_info efifb_dmi_list[];