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

Hi

Am 11.05.22 um 13:24 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.

I find it very hard to review these patches, as related things are put into separate patches.

I suggest to put the sysfb_disable() stuff into patch 5 and the rest into patch 4.

Best regards Thomas

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

Changes in v4:

• Make sysfb_disable() to also attempt to unregister a device.

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 | 87 +++++++++++++++++-- include/linux/sysfb.h | 19 ++++ 3 files changed, 106 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..6768968949e6 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -34,21 +34,92 @@ #include <linux/screen_info.h> #include <linux/sysfb.h>

+static struct platform_device *pd; +static DEFINE_MUTEX(disable_lock); +static bool disabled;

+static bool sysfb_unregister(void) +{

• if (IS_ERR_OR_NULL(pd))

• return false;
  

• platform_device_unregister(pd);
• pd = NULL;
• return true;

+}

+/**

• • 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.
• • It also unregisters a device if this was already registered by sysfb_init().
• • Context: The function can sleep. A @disable_lock mutex is acquired to serialize

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

• •      sysfb_try_unregister(), that tries to unregister a framebuffer device.
    

• */

+void sysfb_disable(void) +{

• mutex_lock(&disable_lock);
• sysfb_unregister();
• disabled = true;
• mutex_unlock(&disable_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 = false;
• mutex_lock(&disable_lock);
• if (IS_ERR_OR_NULL(pd) || pd != to_platform_device(dev))

• goto unlock_mutex;
  

• ret = sysfb_unregister();

+unlock_mutex:

• mutex_unlock(&disable_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(&disable_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 +131,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 +146,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(&disable_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[];