linux-kernel - Re: [PATCH 01/26] drm: modeset-helper: Export dev_pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <39a93eaa-8942-d630-243a-f6e34a16718d@suse.de>
Date:   Tue, 8 Nov 2022 13:10:49 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Paul Cercueil <paul@...pouillou.net>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/26] drm: modeset-helper: Export dev_pm_ops for simple
 drivers

Hi Paul,

thanks for cleaning up. Please see my comments below.

Am 07.11.22 um 18:50 schrieb Paul Cercueil:
> Export a dev_pm_ops meant to be used with simple drivers, which have
> their struct drm_device registered as their struct device's drvdata, and
> only call drm_mode_config_pm_{suspend,resume}.
> 
> The symbol is conditionally exported if IS_ENABLED(CONFIG_PM_SLEEP), and
> therefore it should always be referenced using the pm_sleep_ptr() macro.
> 
> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
> ---
>   drivers/gpu/drm/drm_modeset_helper.c | 32 ++++++++++++++++++++++++++++
>   include/drm/drm_modeset_helper.h     |  4 ++++
>   2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c
> index f858dfedf2cf..0bc9f9228a60 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -20,6 +20,9 @@
>    * OF THIS SOFTWARE.
>    */
>   
> +#include <linux/device.h>
> +#include <linux/pm.h>
> +
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_fourcc.h>
> @@ -244,3 +247,32 @@ int drm_mode_config_helper_resume(struct drm_device *dev)
>   	return ret;
>   }
>   EXPORT_SYMBOL(drm_mode_config_helper_resume);
> +
> +static int drm_mode_config_pm_suspend(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +
> +	return drm_mode_config_helper_suspend(drm);
> +}
> +
> +static int drm_mode_config_pm_resume(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +
> +	return drm_mode_config_helper_resume(drm);
> +}
> +
> +/**
> + * drm_mode_config_pm_ops - Exported dev_pm_ops helper for simple drivers
> + *
> + * This dev_pm_ops can be used for simple drivers that would otherwise only call
> + * drm_mode_config_helper_suspend / drm_mode_config_helper_resume in their PM
> + * callbacks. It is only valid if the driver's drm_device has been registered as
> + * the struct device's drvdata.
> + *
> + * The exported symbol must always be used with the pm_sleep_ptr() macro, like
> + * this:
> + * .pm = pm_sleep_ptr(&drm_mode_config_pm_ops),
> + */
> +EXPORT_SIMPLE_DEV_PM_OPS(drm_mode_config_pm_ops,
> +			 drm_mode_config_pm_suspend, drm_mode_config_pm_resume);
> diff --git a/include/drm/drm_modeset_helper.h b/include/drm/drm_modeset_helper.h
> index 995fd981cab0..85f29637e9c1 100644
> --- a/include/drm/drm_modeset_helper.h
> +++ b/include/drm/drm_modeset_helper.h
> @@ -23,6 +23,8 @@
>   #ifndef __DRM_KMS_HELPER_H__
>   #define __DRM_KMS_HELPER_H__

I like that you clean up the driver, but not how it's done TBH.

>   
> +#include <linux/pm.h>
> +

Half of DRM somehow incudes drm_kms_helper.h. So this include statements 
affects more or less everything.

>   struct drm_crtc;
>   struct drm_crtc_funcs;
>   struct drm_device;
> @@ -41,4 +43,6 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>   int drm_mode_config_helper_suspend(struct drm_device *dev);
>   int drm_mode_config_helper_resume(struct drm_device *dev);
>   
> +extern const struct dev_pm_ops drm_mode_config_pm_ops;
> +

That's maybe subjective, but I don't like exporting such _funcs and _ops 
instances. They are like blackboxes.  And they pollute the symbol 
namespace unnecessarily.

I propose a solution similar to DEFINE_DRM_GEM_FOPS [1] or 
drm_module_pci_driver. [2]

Define a macro in the header to create the _ops instance, such as

#if defined(CONFIG_PM)
#define DEFINE_DRM_MODE_CONFIG_HELPER_PM_OPS(_name)  \
   static __ ## _name ## _suspend() {  \
     call drm_mode_config_helper_suspend()  \
   } \
   static __ ## _name ## _resume() {  \
     call drm_mode_config_helper_resume()  \
   }  \
   static SIMPLE_DEV_PM_OPS(_name, __ ## _name ## _suspend, __ ## _name 
## _resume);
#else
#define DEFINE_DRM_MODE_CONFIG_HELPER_PM_OPS(_name)
#endif

Drivers can then keep the instance and the include for pm.h internal 
within their _drv.c file. The small callback functions are within the 
source file as well.  If CONFIG_PM has been disabled, nothing is being 
generated.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_gem.h#L396
[2] 
https://elixir.bootlin.com/linux/v6.1-rc4/source/include/drm/drm_module.h#L58


>   #endif

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)