The DRM-managed function to register a CRTC is drmm_crtc_alloc_with_planes(), which will allocate the underlying structure and initialisation the CRTC.

However, we might want to separate the structure creation and the CRTC initialisation, for example if the structure is shared across multiple DRM entities, for example an encoder and a connector.

Let's create an helper to only initialise a CRTC that would be passed as an argument.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_crtc.c | 75 ++++++++++++++++++++++++++++++++------ include/drm/drm_crtc.h | 6 +++ 2 files changed, 69 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 26a77a735905..6c1fb42ecf19 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -341,9 +341,10 @@ static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc * * The @primary and @cursor planes are only relevant for legacy uAPI, see * &drm_crtc.primary and &drm_crtc.cursor. * - * Note: consider using drmm_crtc_alloc_with_planes() instead of - * drm_crtc_init_with_planes() to let the DRM managed resource infrastructure - * take care of cleanup and deallocation. + * Note: consider using drmm_crtc_alloc_with_planes() or + * drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes() + * to let the DRM managed resource infrastructure take care of cleanup + * and deallocation. * * Returns: * Zero on success, error code on failure. @@ -368,14 +369,69 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, } EXPORT_SYMBOL(drm_crtc_init_with_planes);

-static void drmm_crtc_alloc_with_planes_cleanup(struct drm_device *dev, - void *ptr) +static void drmm_crtc_init_with_planes_cleanup(struct drm_device *dev, + void *ptr) { struct drm_crtc *crtc = ptr;

drm_crtc_cleanup(crtc); }

+/** + * drmm_crtc_init_with_planes - Initialise a new CRTC object with + * specified primary and cursor planes. + * @dev: DRM device + * @crtc: CRTC object to init + * @primary: Primary plane for CRTC + * @cursor: Cursor plane for CRTC + * @funcs: callbacks for the new CRTC + * @name: printf style format string for the CRTC name, or NULL for default name + * + * Inits a new object created as base part of a driver crtc object. Drivers + * should use this function instead of drm_crtc_init(), which is only provided + * for backwards compatibility with drivers which do not yet support universal + * planes). For really simple hardware which has only 1 plane look at + * drm_simple_display_pipe_init() instead. + * + * Cleanup is automatically handled through registering + * drmm_crtc_cleanup() with drmm_add_action(). The crtc structure should + * be allocated with drmm_kzalloc(). + * + * The @drm_crtc_funcs.destroy hook must be NULL. + * + * The @primary and @cursor planes are only relevant for legacy uAPI, see + * &drm_crtc.primary and &drm_crtc.cursor. + * + * Returns: + * Zero on success, error code on failure. + */ +int drmm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, + struct drm_plane *primary, + struct drm_plane *cursor, + const struct drm_crtc_funcs *funcs, + const char *name, ...) +{ + va_list ap; + int ret; + + WARN_ON(funcs && funcs->destroy); + + va_start(ap, name); + ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs, + name, ap); + va_end(ap); + if (ret) + return ret; + + ret = drmm_add_action_or_reset(dev, drmm_crtc_init_with_planes_cleanup, + crtc); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL(drmm_crtc_init_with_planes); + void *__drmm_crtc_alloc_with_planes(struct drm_device *dev, size_t size, size_t offset, struct drm_plane *primary, @@ -398,17 +454,12 @@ void *__drmm_crtc_alloc_with_planes(struct drm_device *dev, crtc = container + offset;

va_start(ap, name); - ret = __drm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs, - name, ap); + ret = drmm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs, + name, ap); va_end(ap); if (ret) return ERR_PTR(ret);

- ret = drmm_add_action_or_reset(dev, drmm_crtc_alloc_with_planes_cleanup, - crtc); - if (ret) - return ERR_PTR(ret); - return container; } EXPORT_SYMBOL(__drmm_crtc_alloc_with_planes); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a70baea0636c..2babd5cffbf3 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1229,6 +1229,12 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_plane *cursor, const struct drm_crtc_funcs *funcs, const char *name, ...); +int drmm_crtc_init_with_planes(struct drm_device *dev, + struct drm_crtc *crtc, + struct drm_plane *primary, + struct drm_plane *cursor, + const struct drm_crtc_funcs *funcs, + const char *name, ...); void drm_crtc_cleanup(struct drm_crtc *crtc);

__printf(7, 8)

The DRM-managed function to register an encoder is drmm_encoder_alloc() and its variants, which will allocate the underlying structure and initialisation the encoder.

However, we might want to separate the structure creation and the encoder initialisation, for example if the structure is shared across multiple DRM entities, for example an encoder and a connector.

Let's create an helper to only initialise an encoder that would be passed as an argument.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_encoder.c | 57 ++++++++++++++++++++++++++++------- include/drm/drm_encoder.h | 5 +++ 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index a940024c8087..b29f2f07744f 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -148,9 +148,9 @@ static int __drm_encoder_init(struct drm_device *dev, * the encoder structure. The encoder structure should not be allocated with * devm_kzalloc(). * - * Note: consider using drmm_encoder_alloc() instead of drm_encoder_init() to - * let the DRM managed resource infrastructure take care of cleanup and - * deallocation. + * Note: consider using drmm_encoder_alloc() or drmm_encoder_init() + * instead of drm_encoder_init() to let the DRM managed resource + * infrastructure take care of cleanup and deallocation. * * Returns: * Zero on success, error code on failure. @@ -221,9 +221,6 @@ void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset, va_list ap; int ret;

- if (WARN_ON(funcs && funcs->destroy)) - return ERR_PTR(-EINVAL); - container = drmm_kzalloc(dev, size, GFP_KERNEL); if (!container) return ERR_PTR(-ENOMEM); @@ -231,19 +228,57 @@ void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset, encoder = container + offset;

va_start(ap, name); - ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); + ret = drmm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); va_end(ap); if (ret) return ERR_PTR(ret);

- ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder); - if (ret) - return ERR_PTR(ret); - return container; } EXPORT_SYMBOL(__drmm_encoder_alloc);

+/** + * drmm_encoder_init - Initialize a preallocated encoder + * @dev: drm device + * @encoder: the encoder to init + * @funcs: callbacks for this encoder (optional) + * @encoder_type: user visible type of the encoder + * @name: printf style format string for the encoder name, or NULL for default name + * + * Initializes a preallocated encoder. Encoder should be subclassed as + * part of driver encoder objects. Cleanup is automatically handled + * through registering drm_encoder_cleanup() with drmm_add_action(). The + * encoder structure should be allocated with drmm_kzalloc(). + * + * The @drm_encoder_funcs.destroy hook must be NULL. + * + * Returns: + * Zero on success, error code on failure. + */ +int drmm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type, const char *name, ...) +{ + va_list ap; + int ret; + + if (WARN_ON(funcs && funcs->destroy)) + return -EINVAL; + + va_start(ap, name); + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); + va_end(ap); + if (ret) + return ret; + + ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL(drmm_encoder_init); + static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder) { struct drm_connector *connector; diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 6e91a0280f31..6713fe1804e9 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -194,6 +194,11 @@ int drm_encoder_init(struct drm_device *dev, const struct drm_encoder_funcs *funcs, int encoder_type, const char *name, ...);

+int drmm_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type, const char *name, ...); + __printf(6, 7) void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,

Unlike most of the other files in DRM, and Linux in general, the headers in drm_connector.c aren't sorted alphabetically. Let's fix that.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_connector.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 1c48d162c77e..353d83ae09d3 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -22,14 +22,14 @@

#include <drm/drm_auth.h> #include <drm/drm_connector.h> +#include <drm/drm_drv.h> #include <drm/drm_edid.h> #include <drm/drm_encoder.h> -#include <drm/drm_utils.h> -#include <drm/drm_print.h> -#include <drm/drm_drv.h> #include <drm/drm_file.h> +#include <drm/drm_print.h> #include <drm/drm_privacy_screen_consumer.h> #include <drm/drm_sysfs.h> +#include <drm/drm_utils.h>

#include <linux/uaccess.h>

The current documentation for drm_connector_unregister() mentions that it's needed for connectors that have been registered through drm_dev_register().

However, this was a typo and was meant to be drm_connector_register(), which only applies to connectors registered after drm_dev_register() has been called.

In addition, it was also mentioning that connectors are unregistered automatically when drm_dev_unregister() is called. This part is a bit misleading, since it might make it appear that drm_connector_unregister() applies either to all connectors, or none of them.

After discussing it with Daniel, it appears that we always need to call drm_connector_unregister() on connectors that have been registered with drm_connector_register(), but only those.

drm_connector_init() already mentions that it only needs drm_connector_cleanup(), so let's clarify the drm_connector_register() and drm_connector_unregister() documentation to point at each other, and remove the misleading part about drm_dev_unregister().

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_connector.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index f0c4665caf38..b9f22cc4ee67 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -524,6 +524,9 @@ EXPORT_SYMBOL(drm_connector_cleanup); * e.g. DP MST connectors. All other connectors will be registered automatically * when calling drm_dev_register(). * + * When the connector is no longer available, callers must call + * drm_connector_unregister(). + * * Returns: * Zero on success, error code on failure. */ @@ -580,9 +583,8 @@ EXPORT_SYMBOL(drm_connector_register); * @connector: the connector to unregister * * Unregister userspace interfaces for a connector. Only call this for - * connectors which have registered explicitly by calling drm_dev_register(), - * since connectors are unregistered automatically when drm_dev_unregister() is - * called. + * connectors which have been registered explicitly by calling + * drm_connector_register(). */ void drm_connector_unregister(struct drm_connector *connector) {

Let's create a DRM-managed variant of drm_connector_init_with_ddc that will take care of an action of the connector cleanup.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_connector.c | 74 ++++++++++++++++++++++++++++----- include/drm/drm_connector.h | 5 +++ 2 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 0fec2d87178f..076ca247c6d0 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -354,6 +354,30 @@ int drm_connector_init(struct drm_device *dev, } EXPORT_SYMBOL(drm_connector_init);

+typedef int (*connector_init_t)(struct drm_device *dev, + struct drm_connector *connector, + const struct drm_connector_funcs *funcs, + int connector_type); + +static int __drm_connector_init_with_ddc(struct drm_device *dev, + struct drm_connector *connector, + connector_init_t init_func, + const struct drm_connector_funcs *funcs, + int connector_type, + struct i2c_adapter *ddc) +{ + int ret; + + ret = init_func(dev, connector, funcs, connector_type); + if (ret) + return ret; + + /* provide ddc symlink in sysfs */ + connector->ddc = ddc; + + return ret; +} + /** * drm_connector_init_with_ddc - Init a preallocated connector * @dev: DRM device @@ -371,6 +395,10 @@ EXPORT_SYMBOL(drm_connector_init); * * Ensures that the ddc field of the connector is correctly set. * + * Note: consider using drmm_connector_init_with_ddc() instead of + * drm_connector_init_with_ddc() to let the DRM managed resource + * infrastructure take care of cleanup and deallocation. + * * Returns: * Zero on success, error code on failure. */ @@ -380,16 +408,9 @@ int drm_connector_init_with_ddc(struct drm_device *dev, int connector_type, struct i2c_adapter *ddc) { - int ret; - - ret = drm_connector_init(dev, connector, funcs, connector_type); - if (ret) - return ret; - - /* provide ddc symlink in sysfs */ - connector->ddc = ddc; - - return ret; + return __drm_connector_init_with_ddc(dev, connector, + drm_connector_init, + funcs, connector_type, ddc); } EXPORT_SYMBOL(drm_connector_init_with_ddc);

@@ -443,6 +464,39 @@ int drmm_connector_init(struct drm_device *dev, } EXPORT_SYMBOL(drmm_connector_init);

+/** + * drmm_connector_init_with_ddc - Init a preallocated connector + * @dev: DRM device + * @connector: the connector to init + * @funcs: callbacks for this connector + * @connector_type: user visible type of the connector + * @ddc: pointer to the associated ddc adapter + * + * Initialises a preallocated connector. Connectors should be + * subclassed as part of driver connector objects. + * + * Cleanup is automatically handled with a call to + * drm_connector_cleanup() in a DRM-managed action. + * + * The connector structure should be allocated with drmm_kzalloc(). + * + * Ensures that the ddc field of the connector is correctly set. + * + * Returns: + * Zero on success, error code on failure. + */ +int drmm_connector_init_with_ddc(struct drm_device *dev, + struct drm_connector *connector, + const struct drm_connector_funcs *funcs, + int connector_type, + struct i2c_adapter *ddc) +{ + return __drm_connector_init_with_ddc(dev, connector, + drmm_connector_init, + funcs, connector_type, ddc); +} +EXPORT_SYMBOL(drmm_connector_init_with_ddc); + /** * drm_connector_attach_edid_property - attach edid property. * @connector: the connector diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 35a6b6e944b7..2565541f2c10 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1676,6 +1676,11 @@ int drmm_connector_init(struct drm_device *dev, struct drm_connector *connector, const struct drm_connector_funcs *funcs, int connector_type); +int drmm_connector_init_with_ddc(struct drm_device *dev, + struct drm_connector *connector, + const struct drm_connector_funcs *funcs, + int connector_type, + struct i2c_adapter *ddc); void drm_connector_attach_edid_property(struct drm_connector *connector); int drm_connector_register(struct drm_connector *connector); void drm_connector_unregister(struct drm_connector *connector);

Unlike what can be found for other DRM entities, we don't have a DRM-managed function equivalent to devm_drm_of_get_bridge().

Let's create it.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/bridge/panel.c | 35 ++++++++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 2 ++ 2 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 07d720aa38c6..0bf824ca1f25 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -425,4 +425,39 @@ struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, return bridge; } EXPORT_SYMBOL(devm_drm_of_get_bridge); + +/** + * drmm_of_get_bridge - Return next bridge in the chain + * @dev: device to tie the bridge lifetime to + * @np: device tree node containing encoder output ports + * @port: port in the device tree node + * @endpoint: endpoint in the device tree node + * + * Given a DT node's port and endpoint number, finds the connected node + * and returns the associated bridge if any, or creates and returns a + * drm panel bridge instance if a panel is connected. + * + * Returns a pointer to the bridge if successful, or an error pointer + * otherwise. + */ +struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, + struct device_node *np, + u32 port, u32 endpoint) +{ + struct drm_bridge *bridge; + struct drm_panel *panel; + int ret; + + ret = drm_of_find_panel_or_bridge(np, port, endpoint, + &panel, &bridge); + if (ret) + return ERR_PTR(ret); + + if (panel) + bridge = drmm_panel_bridge_add(drm, panel); + + return bridge; +} +EXPORT_SYMBOL(drmm_of_get_bridge); + #endif diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 8100a15dd9c2..ddb92e745b2e 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -935,6 +935,8 @@ struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge); #if defined(CONFIG_OF) && defined(CONFIG_DRM_PANEL_BRIDGE) struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node, u32 port, u32 endpoint); +struct drm_bridge *drmm_of_get_bridge(struct drm_device *drm, struct device_node *node, + u32 port, u32 endpoint); #else static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,

When our KMS driver is unbound, the device is no longer there but we might still have users with an opened fd to the KMS device.

To avoid any issue in such a situation, every device access needs to be protected by calls to drm_dev_enter() and drm_dev_exit(), and the driver needs to call drm_dev_unplug().

We'll add calls to drm_dev_enter()/drm_dev_exit() in subsequent patches changing the relevant drivers, but let's start by calling drm_dev_unplug().

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_drv.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 90575171824d..06d605a12633 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -380,8 +380,7 @@ static void vc4_drm_unbind(struct device *dev) { struct drm_device *drm = dev_get_drvdata(dev);

- drm_dev_unregister(drm); - + drm_dev_unplug(drm); drm_atomic_helper_shutdown(drm); }

Whenever the device and driver are unbound, the main device and all the subdevices will be removed by calling their unbind() method.

However, the DRM device itself will only be freed when the last user will have closed it.

It means that there is a time window where the device and its resources aren't there anymore, but the userspace can still call into our driver.

Fortunately, the DRM framework provides the drm_dev_enter() and drm_dev_exit() functions to make sure our underlying device is still there for the section protected by those calls. Let's add them to the HVS driver.

Reviewed-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hvs.c | 99 ++++++++++++++++++++++++++++++++--- 1 file changed, 93 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c index ba2c8e5a9b64..c02243bcb61f 100644 --- a/drivers/gpu/drm/vc4/vc4_hvs.c +++ b/drivers/gpu/drm/vc4/vc4_hvs.c @@ -25,6 +25,7 @@ #include <linux/platform_device.h>

#include <drm/drm_atomic_helper.h> +#include <drm/drm_drv.h> #include <drm/drm_vblank.h>

#include "vc4_drv.h" @@ -66,11 +67,15 @@ static const struct debugfs_reg32 hvs_regs[] = {

void vc4_hvs_dump_state(struct vc4_hvs *hvs) { + struct drm_device *drm = &hvs->vc4->base; struct drm_printer p = drm_info_printer(&hvs->pdev->dev); - int i; + int idx, i;

drm_print_regset32(&p, &hvs->regset);

+ if (!drm_dev_enter(drm, &idx)) + return; + DRM_INFO("HVS ctx:\n"); for (i = 0; i < 64; i += 4) { DRM_INFO("0x%08x (%s): 0x%08x 0x%08x 0x%08x 0x%08x\n", @@ -80,6 +85,8 @@ void vc4_hvs_dump_state(struct vc4_hvs *hvs) readl((u32 __iomem *)hvs->dlist + i + 2), readl((u32 __iomem *)hvs->dlist + i + 3)); } + + drm_dev_exit(idx); }

static int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data) @@ -135,6 +142,11 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs, int ret, i; u32 __iomem *dst_kernel;

+ /* + * NOTE: We don't need a call to drm_dev_enter()/drm_dev_exit() + * here since that function is only called from vc4_hvs_bind(). + */ + ret = drm_mm_insert_node(&hvs->dlist_mm, space, VC4_KERNEL_DWORDS); if (ret) { DRM_ERROR("Failed to allocate space for filter kernel: %d\n", @@ -159,10 +171,15 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs *hvs, static void vc4_hvs_lut_load(struct vc4_hvs *hvs, struct vc4_crtc *vc4_crtc) { + struct drm_device *drm = &hvs->vc4->base; struct drm_crtc *crtc = &vc4_crtc->base; struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + int idx; u32 i;

+ if (!drm_dev_enter(drm, &idx)) + return; + /* The LUT memory is laid out with each HVS channel in order, * each of which takes 256 writes for R, 256 for G, then 256 * for B. @@ -177,6 +194,8 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs, HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_g[i]); for (i = 0; i < crtc->gamma_size; i++) HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]); + + drm_dev_exit(idx); }

static void vc4_hvs_update_gamma_lut(struct vc4_hvs *hvs, @@ -198,7 +217,12 @@ static void vc4_hvs_update_gamma_lut(struct vc4_hvs *hvs,

u8 vc4_hvs_get_fifo_frame_count(struct vc4_hvs *hvs, unsigned int fifo) { + struct drm_device *drm = &hvs->vc4->base; u8 field = 0; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return 0;

switch (fifo) { case 0: @@ -215,6 +239,7 @@ u8 vc4_hvs_get_fifo_frame_count(struct vc4_hvs *hvs, unsigned int fifo) break; }

+ drm_dev_exit(idx); return field; }

@@ -227,6 +252,12 @@ int vc4_hvs_get_fifo_from_output(struct vc4_hvs *hvs, unsigned int output) if (!vc4->is_vc5) return output;

+ /* + * NOTE: We should probably use drm_dev_enter()/drm_dev_exit() + * here, but this function is only used during the DRM device + * initialization, so we should be fine. + */ + switch (output) { case 0: return 0; @@ -275,12 +306,17 @@ static int vc4_hvs_init_channel(struct vc4_hvs *hvs, struct drm_crtc *crtc, struct drm_display_mode *mode, bool oneshot) { struct vc4_dev *vc4 = hvs->vc4; + struct drm_device *drm = &vc4->base; struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc); struct vc4_crtc_state *vc4_crtc_state = to_vc4_crtc_state(crtc->state); unsigned int chan = vc4_crtc_state->assigned_channel; bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE; u32 dispbkgndx; u32 dispctrl; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return -ENODEV;

HVS_WRITE(SCALER_DISPCTRLX(chan), 0); HVS_WRITE(SCALER_DISPCTRLX(chan), SCALER_DISPCTRLX_RESET); @@ -322,13 +358,21 @@ static int vc4_hvs_init_channel(struct vc4_hvs *hvs, struct drm_crtc *crtc, */ vc4_hvs_lut_load(hvs, vc4_crtc);

+ drm_dev_exit(idx); + return 0; }

void vc4_hvs_stop_channel(struct vc4_hvs *hvs, unsigned int chan) { + struct drm_device *drm = &hvs->vc4->base; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return; + if (HVS_READ(SCALER_DISPCTRLX(chan)) & SCALER_DISPCTRLX_ENABLE) - return; + goto out;

HVS_WRITE(SCALER_DISPCTRLX(chan), HVS_READ(SCALER_DISPCTRLX(chan)) | SCALER_DISPCTRLX_RESET); @@ -345,6 +389,9 @@ void vc4_hvs_stop_channel(struct vc4_hvs *hvs, unsigned int chan) WARN_ON_ONCE((HVS_READ(SCALER_DISPSTATX(chan)) & (SCALER_DISPSTATX_FULL | SCALER_DISPSTATX_EMPTY)) != SCALER_DISPSTATX_EMPTY); + +out: + drm_dev_exit(idx); }

int vc4_hvs_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) @@ -386,9 +433,15 @@ static void vc4_hvs_install_dlist(struct drm_crtc *crtc) struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_hvs *hvs = vc4->hvs; struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state); + int idx; + + if (!drm_dev_enter(dev, &idx)) + return;

HVS_WRITE(SCALER_DISPLISTX(vc4_state->assigned_channel), vc4_state->mm.start); + + drm_dev_exit(idx); }

static void vc4_hvs_update_dlist(struct drm_crtc *crtc) @@ -473,6 +526,12 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc, bool enable_bg_fill = false; u32 __iomem *dlist_start = vc4->hvs->dlist + vc4_state->mm.start; u32 __iomem *dlist_next = dlist_start; + int idx; + + if (!drm_dev_enter(dev, &idx)) { + vc4_crtc_send_vblank(crtc); + return; + }

if (debug_dump_regs) { DRM_INFO("CRTC %d HVS before:\n", drm_crtc_index(crtc)); @@ -543,26 +602,44 @@ void vc4_hvs_atomic_flush(struct drm_crtc *crtc, DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc)); vc4_hvs_dump_state(hvs); } + + drm_dev_exit(idx); }

void vc4_hvs_mask_underrun(struct vc4_hvs *hvs, int channel) { - u32 dispctrl = HVS_READ(SCALER_DISPCTRL); + struct drm_device *drm = &hvs->vc4->base; + u32 dispctrl; + int idx;

+ if (!drm_dev_enter(drm, &idx)) + return; + + dispctrl = HVS_READ(SCALER_DISPCTRL); dispctrl &= ~SCALER_DISPCTRL_DSPEISLUR(channel);

HVS_WRITE(SCALER_DISPCTRL, dispctrl); + + drm_dev_exit(idx); }

void vc4_hvs_unmask_underrun(struct vc4_hvs *hvs, int channel) { - u32 dispctrl = HVS_READ(SCALER_DISPCTRL); + struct drm_device *drm = &hvs->vc4->base; + u32 dispctrl; + int idx;

+ if (!drm_dev_enter(drm, &idx)) + return; + + dispctrl = HVS_READ(SCALER_DISPCTRL); dispctrl |= SCALER_DISPCTRL_DSPEISLUR(channel);

HVS_WRITE(SCALER_DISPSTAT, SCALER_DISPSTAT_EUFLOW(channel)); HVS_WRITE(SCALER_DISPCTRL, dispctrl); + + drm_dev_exit(idx); }

static void vc4_hvs_report_underrun(struct drm_device *dev) @@ -583,6 +660,17 @@ static irqreturn_t vc4_hvs_irq_handler(int irq, void *data) u32 control; u32 status;

+ /* + * NOTE: We don't need to protect the register access using + * drm_dev_enter() there because the interrupt handler lifetime + * is tied to the device itself, and not to the DRM device. + * + * So when the device will be gone, one of the first thing we + * will be doing will be to unregister the interrupt handler, + * and then unregister the DRM device. drm_dev_enter() would + * thus always succeed if we are here. + */ + status = HVS_READ(SCALER_DISPSTAT); control = HVS_READ(SCALER_DISPCTRL);

@@ -615,10 +703,9 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data) u32 dispctrl; u32 reg;

- hvs = devm_kzalloc(&pdev->dev, sizeof(*hvs), GFP_KERNEL); + hvs = drmm_kzalloc(drm, sizeof(*hvs), GFP_KERNEL); if (!hvs) return -ENOMEM; - hvs->vc4 = vc4; hvs->pdev = pdev;

vc4_plane_init() currently initialises the plane with no possible CRTCs, and will expect the caller to set it up by itself.

Let's change that logic a bit to follow the syntax of drm_universal_plane_init() and pass the possible CRTCs bitmask as an argument to the function instead.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- drivers/gpu/drm/vc4/vc4_drv.h | 3 ++- drivers/gpu/drm/vc4/vc4_plane.c | 15 +++++++-------- 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 49ff0506e551..32c0dd6a7348 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -1239,7 +1239,7 @@ int vc4_crtc_init(struct drm_device *drm, struct vc4_crtc *vc4_crtc, * requirement of the plane configuration, and reject ones * that will take too much. */ - primary_plane = vc4_plane_init(drm, DRM_PLANE_TYPE_PRIMARY); + primary_plane = vc4_plane_init(drm, DRM_PLANE_TYPE_PRIMARY, 0); if (IS_ERR(primary_plane)) { dev_err(drm->dev, "failed to construct primary plane\n"); return PTR_ERR(primary_plane); diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index da52d1e0c7b5..91983fbd4d83 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -959,7 +959,8 @@ int vc4_kms_load(struct drm_device *dev);

/* vc4_plane.c */ struct drm_plane *vc4_plane_init(struct drm_device *dev, - enum drm_plane_type type); + enum drm_plane_type type, + uint32_t possible_crtcs); int vc4_plane_create_additional_planes(struct drm_device *dev); u32 vc4_plane_write_dlist(struct drm_plane *plane, u32 __iomem *dlist); u32 vc4_plane_dlist_size(const struct drm_plane_state *state); diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 1e866dc00ac3..bc1f1c4172e7 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -1462,7 +1462,8 @@ static const struct drm_plane_funcs vc4_plane_funcs = { };

struct drm_plane *vc4_plane_init(struct drm_device *dev, - enum drm_plane_type type) + enum drm_plane_type type, + uint32_t possible_crtcs) { struct vc4_dev *vc4 = to_vc4_dev(dev); struct drm_plane *plane = NULL; @@ -1493,7 +1494,7 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev, }

plane = &vc4_plane->base; - ret = drm_universal_plane_init(dev, plane, 0, + ret = drm_universal_plane_init(dev, plane, possible_crtcs, &vc4_plane_funcs, formats, num_formats, modifiers, type, NULL); @@ -1541,13 +1542,11 @@ int vc4_plane_create_additional_planes(struct drm_device *drm) */ for (i = 0; i < 16; i++) { struct drm_plane *plane = - vc4_plane_init(drm, DRM_PLANE_TYPE_OVERLAY); + vc4_plane_init(drm, DRM_PLANE_TYPE_OVERLAY, + GENMASK(drm->mode_config.num_crtc - 1, 0));

if (IS_ERR(plane)) continue; - - plane->possible_crtcs = - GENMASK(drm->mode_config.num_crtc - 1, 0); }

drm_for_each_crtc(crtc, drm) { @@ -1555,9 +1554,9 @@ int vc4_plane_create_additional_planes(struct drm_device *drm) * since we overlay planes on the CRTC in the order they were * initialized. */ - cursor_plane = vc4_plane_init(drm, DRM_PLANE_TYPE_CURSOR); + cursor_plane = vc4_plane_init(drm, DRM_PLANE_TYPE_CURSOR, + drm_crtc_mask(crtc)); if (!IS_ERR(cursor_plane)) { - cursor_plane->possible_crtcs = drm_crtc_mask(crtc); crtc->cursor = cursor_plane; } }

Let's switch to drmm_universal_plane_alloc() for our plane allocation and initialisation to make the driver a bit simpler.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 1 - drivers/gpu/drm/vc4/vc4_plane.c | 23 ++++++++--------------- 2 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index e559fdb217a5..84fd25e71c41 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -1277,7 +1277,6 @@ static int vc4_crtc_bind(struct device *dev, struct device *master, void *data) const struct vc4_pv_data *pv_data; struct vc4_crtc *vc4_crtc; struct drm_crtc *crtc; - struct drm_plane *destroy_plane, *temp; int ret;

vc4_crtc = devm_kzalloc(dev, sizeof(*vc4_crtc), GFP_KERNEL); diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index bc1f1c4172e7..813ed4c05b29 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -1453,8 +1453,6 @@ static bool vc4_format_mod_supported(struct drm_plane *plane, static const struct drm_plane_funcs vc4_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, - .destroy = drm_plane_cleanup, - .set_property = NULL, .reset = vc4_plane_reset, .atomic_duplicate_state = vc4_plane_duplicate_state, .atomic_destroy_state = vc4_plane_destroy_state, @@ -1466,11 +1464,10 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev, uint32_t possible_crtcs) { struct vc4_dev *vc4 = to_vc4_dev(dev); - struct drm_plane *plane = NULL; + struct drm_plane *plane; struct vc4_plane *vc4_plane; u32 formats[ARRAY_SIZE(hvs_formats)]; int num_formats = 0; - int ret = 0; unsigned i; static const uint64_t modifiers[] = { DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED, @@ -1481,11 +1478,6 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev, DRM_FORMAT_MOD_INVALID };

- vc4_plane = devm_kzalloc(dev->dev, sizeof(*vc4_plane), - GFP_KERNEL); - if (!vc4_plane) - return ERR_PTR(-ENOMEM); - for (i = 0; i < ARRAY_SIZE(hvs_formats); i++) { if (!hvs_formats[i].hvs5_only || vc4->is_vc5) { formats[num_formats] = hvs_formats[i].drm; @@ -1493,13 +1485,14 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev, } }

+ vc4_plane = drmm_universal_plane_alloc(dev, struct vc4_plane, base, + possible_crtcs, + &vc4_plane_funcs, + formats, num_formats, + modifiers, type, NULL); + if (IS_ERR(vc4_plane)) + return ERR_CAST(vc4_plane); plane = &vc4_plane->base; - ret = drm_universal_plane_init(dev, plane, possible_crtcs, - &vc4_plane_funcs, - formats, num_formats, - modifiers, type, NULL); - if (ret) - return ERR_PTR(ret);

if (vc4->is_vc5) drm_plane_helper_add(plane, &vc5_plane_helper_funcs);

Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc.

This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run.

However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free.

Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c index 188cf25125f0..dadf962b0bb5 100644 --- a/drivers/gpu/drm/vc4/vc4_crtc.c +++ b/drivers/gpu/drm/vc4/vc4_crtc.c @@ -1279,7 +1279,7 @@ static int vc4_crtc_bind(struct device *dev, struct device *master, void *data) struct drm_crtc *crtc; int ret;

- vc4_crtc = devm_kzalloc(dev, sizeof(*vc4_crtc), GFP_KERNEL); + vc4_crtc = drmm_kzalloc(drm, sizeof(*vc4_crtc), GFP_KERNEL); if (!vc4_crtc) return -ENOMEM; crtc = &vc4_crtc->base;

There's no user for that pointer so let's just get rid of it.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dpi.c | 7 ------- drivers/gpu/drm/vc4/vc4_drv.h | 1 - 2 files changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index c180eb60bee8..f2b46c524919 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -249,7 +249,6 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); struct drm_device *drm = dev_get_drvdata(master); - struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dpi *dpi; struct vc4_dpi_encoder *vc4_dpi_encoder; int ret; @@ -308,8 +307,6 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data)

dev_set_drvdata(dev, dpi);

- vc4->dpi = dpi; - vc4_debugfs_add_regset32(drm, "dpi_regs", &dpi->regset);

return 0; @@ -323,8 +320,6 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) static void vc4_dpi_unbind(struct device *dev, struct device *master, void *data) { - struct drm_device *drm = dev_get_drvdata(master); - struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_dpi *dpi = dev_get_drvdata(dev);

drm_of_panel_bridge_remove(dev->of_node, 0, 0); @@ -332,8 +327,6 @@ static void vc4_dpi_unbind(struct device *dev, struct device *master, drm_encoder_cleanup(dpi->encoder);

clk_disable_unprepare(dpi->core_clock); - - vc4->dpi = NULL; }

static const struct component_ops vc4_dpi_ops = { diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index 6afa873f0def..db51dd3e20b8 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -83,7 +83,6 @@ struct vc4_dev {

struct vc4_hvs *hvs; struct vc4_v3d *v3d; - struct vc4_dpi *dpi; struct vc4_vec *vec; struct vc4_txp *txp;

Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc.

This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run.

However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free.

Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dpi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index c88e8e397730..d1eaafb43bd1 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -244,9 +244,10 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) struct vc4_dpi *dpi; int ret;

- dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL); + dpi = drmm_kzalloc(drm, sizeof(*dpi), GFP_KERNEL); if (!dpi) return -ENOMEM; + dpi->encoder.type = VC4_ENCODER_TYPE_DPI; dpi->pdev = pdev; dpi->regs = vc4_ioremap_regs(pdev, 0);

Since we have a managed call to create our panel_bridge instance, the call to drm_of_panel_bridge_remove() at unbind is both redundant and dangerous since it might lead to a use-after-free.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dpi.c | 2 -- 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 658e0aa9e2e1..5a6cdea7bf7b 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -309,8 +309,6 @@ static void vc4_dpi_unbind(struct device *dev, struct device *master, { struct vc4_dpi *dpi = dev_get_drvdata(dev);

- drm_of_panel_bridge_remove(dev->of_node, 0, 0); - drm_encoder_cleanup(&dpi->encoder.base);

clk_disable_unprepare(dpi->core_clock);

The current code will call drm_encoder_cleanup() when the device is unbound. However, by then, there might still be some references held to that encoder, including by the userspace that might still have the DRM device open.

Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dpi.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 4e24dbad77f2..233bc5d3b02f 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -296,35 +296,28 @@ static int vc4_dpi_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;

- drm_simple_encoder_init(drm, &dpi->encoder.base, DRM_MODE_ENCODER_DPI); + ret = drmm_encoder_init(drm, &dpi->encoder.base, + NULL, + DRM_MODE_ENCODER_DPI, + NULL); + if (ret) + return ret; + drm_encoder_helper_add(&dpi->encoder.base, &vc4_dpi_encoder_helper_funcs);

ret = vc4_dpi_init_bridge(dpi); if (ret) - goto err_destroy_encoder; + return ret;

dev_set_drvdata(dev, dpi);

vc4_debugfs_add_regset32(drm, "dpi_regs", &dpi->regset);

return 0; - -err_destroy_encoder: - drm_encoder_cleanup(&dpi->encoder.base); - return ret; -} - -static void vc4_dpi_unbind(struct device *dev, struct device *master, - void *data) -{ - struct vc4_dpi *dpi = dev_get_drvdata(dev); - - drm_encoder_cleanup(&dpi->encoder.base); }

static const struct component_ops vc4_dpi_ops = { .bind = vc4_dpi_bind, - .unbind = vc4_dpi_unbind, };

static int vc4_dpi_dev_probe(struct platform_device *pdev)

Our current code now mixes some resources whose lifetime are tied to the device (clocks, IO mappings, etc.) and some that are tied to the DRM device (encoder, bridge).

The device one will be freed at unbind time, but the DRM one will only be freed when the last user of the DRM device closes its file handle.

So we end up with a time window during which we can call the encoder hooks, but we don't have access to the underlying resources and device.

Let's protect all those sections with drm_dev_enter() and drm_dev_exit() so that we bail out if we are during that window.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dpi.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c index 71bb51e1d703..75b4ea3ab7e3 100644 --- a/drivers/gpu/drm/vc4/vc4_dpi.c +++ b/drivers/gpu/drm/vc4/vc4_dpi.c @@ -13,6 +13,7 @@

#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_drv.h> #include <drm/drm_edid.h> #include <drm/drm_of.h> #include <drm/drm_panel.h> @@ -111,9 +112,16 @@ static const struct debugfs_reg32 dpi_regs[] = {

static void vc4_dpi_encoder_disable(struct drm_encoder *encoder) { + struct drm_device *dev = encoder->dev; struct vc4_dpi *dpi = to_vc4_dpi(encoder); + int idx; + + if (!drm_dev_enter(dev, &idx)) + return;

clk_disable_unprepare(dpi->pixel_clock); + + drm_dev_exit(idx); }

static void vc4_dpi_encoder_enable(struct drm_encoder *encoder) @@ -124,6 +132,7 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder) struct drm_connector_list_iter conn_iter; struct drm_connector *connector = NULL, *connector_scan; u32 dpi_c = DPI_ENABLE | DPI_OUTPUT_ENABLE_MODE; + int idx; int ret;

/* Look up the connector attached to DPI so we can get the @@ -184,6 +193,9 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder) else if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) dpi_c |= DPI_VSYNC_DISABLE;

+ if (!drm_dev_enter(dev, &idx)) + return; + DPI_WRITE(DPI_C, dpi_c);

ret = clk_set_rate(dpi->pixel_clock, mode->clock * 1000); @@ -193,6 +205,8 @@ static void vc4_dpi_encoder_enable(struct drm_encoder *encoder) ret = clk_prepare_enable(dpi->pixel_clock); if (ret) DRM_ERROR("Failed to set clock rate: %d\n", ret); + + drm_dev_exit(idx); }

static enum drm_mode_status vc4_dpi_encoder_mode_valid(struct drm_encoder *encoder,

The current code will call drm_encoder_cleanup() when the device is unbound. However, by then, there might still be some references held to that encoder, including by the userspace that might still have the DRM device open.

Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed.

Reviewed-by: Dave Stevenson dave.stevenson@raspberrypi.com Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dsi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index dbb3f6fb39b4..f3214675f8b3 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -1599,7 +1599,13 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) if (ret) return ret;

- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_DSI); + ret = drmm_encoder_init(drm, encoder, + NULL, + DRM_MODE_ENCODER_DSI, + NULL); + if (ret) + return ret; + drm_encoder_helper_add(encoder, &vc4_dsi_encoder_helper_funcs);

ret = drm_bridge_attach(encoder, dsi->bridge, NULL, 0); @@ -1632,7 +1638,6 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master, * normally. */ list_splice_init(&dsi->bridge_chain, &encoder->bridge_chain); - drm_encoder_cleanup(encoder); }

static const struct component_ops vc4_dsi_ops = {

The vc4_dsi structure is currently allocated through a device-managed allocation. This can lead to use-after-free issues however in the unbinding path since the DRM entities will stick around, but the underlying structure has been freed.

However, we can't just fix it by using a DRM-managed allocation like we did for the other drivers since the DSI case is a bit more intricate.

Indeed, the structure will be allocated at probe time, when we don't have a DRM device yet, to be able to register the DSI bus driver. We will then reuse it at bind time to register our KMS entities in the framework.

In order to work around both constraints, we can use a kref to track the users of the structure (DSI host, and KMS), and then put our structure when the DSI host will have been unregistered, and through a DRM-managed action that will execute once we won't need the KMS entities anymore.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_dsi.c | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c index b3c97aa11e7e..86d53d50ad9d 100644 --- a/drivers/gpu/drm/vc4/vc4_dsi.c +++ b/drivers/gpu/drm/vc4/vc4_dsi.c @@ -510,6 +510,8 @@ struct vc4_dsi { struct vc4_encoder encoder; struct mipi_dsi_host dsi_host;

+ struct kref kref; + struct platform_device *pdev;

struct drm_bridge *bridge; @@ -1479,6 +1481,25 @@ vc4_dsi_init_phy_clocks(struct vc4_dsi *dsi) dsi->clk_onecell); }

+static void vc4_dsi_get(struct vc4_dsi *dsi) +{ + kref_get(&dsi->kref); +} + +static void vc4_dsi_release(struct kref *kref); + +static void vc4_dsi_put(struct vc4_dsi *dsi) +{ + kref_put(&dsi->kref, &vc4_dsi_release); +} + +static void vc4_dsi_release_action(struct drm_device *drm, void *ptr) +{ + struct vc4_dsi *dsi = ptr; + + vc4_dsi_put(dsi); +} + static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); @@ -1488,6 +1509,12 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data) dma_cap_mask_t dma_mask; int ret;

+ vc4_dsi_get(dsi); + + ret = drmm_add_action_or_reset(drm, vc4_dsi_release_action, dsi); + if (ret) + return ret; + dsi->variant = of_device_get_match_data(dev);

INIT_LIST_HEAD(&dsi->bridge_chain); @@ -1645,16 +1672,25 @@ static const struct component_ops vc4_dsi_ops = { .unbind = vc4_dsi_unbind, };

+static void vc4_dsi_release(struct kref *kref) +{ + struct vc4_dsi *dsi = + container_of(kref, struct vc4_dsi, kref); + + kfree(dsi); +} + static int vc4_dsi_dev_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct vc4_dsi *dsi;

- dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); + dsi = kzalloc(sizeof(*dsi), GFP_KERNEL); if (!dsi) return -ENOMEM; dev_set_drvdata(dev, dsi);

+ kref_init(&dsi->kref); dsi->pdev = pdev; dsi->dsi_host.ops = &vc4_dsi_host_ops; dsi->dsi_host.dev = dev; @@ -1669,6 +1705,8 @@ static int vc4_dsi_dev_remove(struct platform_device *pdev) struct vc4_dsi *dsi = dev_get_drvdata(dev);

mipi_dsi_host_unregister(&dsi->dsi_host); + vc4_dsi_put(dsi); + return 0; }

Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc.

This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run.

However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free.

Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 823d812f4982..fd7bfcd1696f 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2834,9 +2834,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct device_node *ddc_node; int ret;

- vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); + vc4_hdmi = drmm_kzalloc(drm, sizeof(*vc4_hdmi), GFP_KERNEL); if (!vc4_hdmi) return -ENOMEM; + mutex_init(&vc4_hdmi->mutex); spin_lock_init(&vc4_hdmi->hw_lock); INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);

The current code will call drm_encoder_cleanup() when the device is unbound. However, by then, there might still be some references held to that encoder, including by the userspace that might still have the DRM device open.

Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 7d08be14b075..592f85993a80 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2916,12 +2916,18 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) clk_prepare_enable(vc4_hdmi->pixel_bvb_clock); }

- drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_TMDS); + ret = drmm_encoder_init(drm, encoder, + NULL, + DRM_MODE_ENCODER_TMDS, + NULL); + if (ret) + goto err_put_runtime_pm; + drm_encoder_helper_add(encoder, &vc4_hdmi_encoder_helper_funcs);

ret = vc4_hdmi_connector_init(drm, vc4_hdmi); if (ret) - goto err_destroy_encoder; + goto err_put_runtime_pm;

ret = vc4_hdmi_hotplug_init(vc4_hdmi); if (ret) @@ -2949,8 +2955,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) vc4_hdmi_hotplug_exit(vc4_hdmi); err_destroy_conn: drm_connector_cleanup(&vc4_hdmi->connector); -err_destroy_encoder: - drm_encoder_cleanup(encoder); +err_put_runtime_pm: pm_runtime_put_sync(dev); pm_runtime_disable(dev); err_put_ddc: @@ -2992,7 +2997,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, vc4_hdmi_cec_exit(vc4_hdmi); vc4_hdmi_hotplug_exit(vc4_hdmi); drm_connector_cleanup(&vc4_hdmi->connector); - drm_encoder_cleanup(&vc4_hdmi->encoder.base);

pm_runtime_disable(dev);

The current code to unregister our ALSA device needs to be undone manually when we remove the HDMI driver.

Since ALSA doesn't seem to support any mechanism to defer freeing something until the last user of the ALSA device is gone, we can either use a device-managed or a DRM-managed action.

The consistent way would be to use a DRM-managed one, just like pretty much any framework-facing structure should be doing. However, ALSA does a lot of allocation and registration using device-managed calls. Thus, if we're going that way, by the time the DRM-managed action would run all of those allocation would have been freed and we would end up with a use-after-free.

Thus, let's do a device-managed action. It's been tested with KASAN enabled and doesn't seem to trigger any issue, so it's as good as anything.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 43 ++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 2d1f87f89994..2de7532b6c78 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2022,6 +2022,14 @@ static struct hdmi_codec_pdata vc4_hdmi_codec_pdata = { .i2s = 1, };

+static void vc4_hdmi_audio_codec_release(void *ptr) +{ + struct vc4_hdmi *vc4_hdmi = ptr; + + platform_device_unregister(vc4_hdmi->audio.codec_pdev); + vc4_hdmi->audio.codec_pdev = NULL; +} + static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) { const struct vc4_hdmi_register *mai_data = @@ -2063,6 +2071,30 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) vc4_hdmi->audio.dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; vc4_hdmi->audio.dma_data.maxburst = 2;

+ /* + * NOTE: Strictly speaking, we should probably use a DRM-managed + * registration there to avoid removing all the audio components + * by the time the driver doesn't have any user anymore. + * + * However, the ASoC core uses a number of devm_kzalloc calls + * when registering, even when using non-device-managed + * functions (such as in snd_soc_register_component()). + * + * If we call snd_soc_unregister_component() in a DRM-managed + * action, the device-managed actions have already been executed + * and thus we would access memory that has been freed. + * + * Using device-managed hooks here probably leaves us open to a + * bunch of issues if userspace still has a handle on the ALSA + * device when the device is removed. However, this is mitigated + * by the use of drm_dev_enter()/drm_dev_exit() in the audio + * path to prevent the access to the device resources if it + * isn't there anymore. + * + * Then, the vc4_hdmi structure is DRM-managed and thus only + * freed whenever the last user has closed the DRM device file. + * It should thus outlive ALSA in most situations. + */ ret = devm_snd_dmaengine_pcm_register(dev, &pcm_conf, 0); if (ret) { dev_err(dev, "Could not register PCM component: %d\n", ret); @@ -2086,6 +2118,10 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi) } vc4_hdmi->audio.codec_pdev = codec_pdev;

+ ret = devm_add_action_or_reset(dev, vc4_hdmi_audio_codec_release, vc4_hdmi); + if (ret) + return ret; + dai_link->cpus = &vc4_hdmi->audio.cpu; dai_link->codecs = &vc4_hdmi->audio.codec; dai_link->platforms = &vc4_hdmi->audio.platform; @@ -2124,12 +2160,6 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)

}

-static void vc4_hdmi_audio_exit(struct vc4_hdmi *vc4_hdmi) -{ - platform_device_unregister(vc4_hdmi->audio.codec_pdev); - vc4_hdmi->audio.codec_pdev = NULL; -} - static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv) { struct vc4_hdmi *vc4_hdmi = priv; @@ -2993,7 +3023,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, kfree(vc4_hdmi->hdmi_regset.regs); kfree(vc4_hdmi->hd_regset.regs);

- vc4_hdmi_audio_exit(vc4_hdmi); vc4_hdmi_cec_exit(vc4_hdmi); vc4_hdmi_hotplug_exit(vc4_hdmi);

The reference to the DDC controller device needs to be put back when we're done with it. Let's use a device-managed action to simplify the driver.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 36ded0f56548..350220786bef 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2858,6 +2858,13 @@ static int vc4_hdmi_runtime_resume(struct device *dev) return 0; }

+static void vc4_hdmi_put_ddc_device(void *ptr) +{ + struct vc4_hdmi *vc4_hdmi = ptr; + + put_device(&vc4_hdmi->ddc->dev); +} + static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) { const struct vc4_hdmi_variant *variant = of_device_get_match_data(dev); @@ -2913,13 +2920,16 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) return -EPROBE_DEFER; }

+ ret = devm_add_action_or_reset(dev, vc4_hdmi_put_ddc_device, vc4_hdmi); + if (ret) + return ret; + /* Only use the GPIO HPD pin if present in the DT, otherwise * we'll use the HDMI core's register. */ vc4_hdmi->hpd_gpio = devm_gpiod_get_optional(dev, "hpd", GPIOD_IN); if (IS_ERR(vc4_hdmi->hpd_gpio)) { - ret = PTR_ERR(vc4_hdmi->hpd_gpio); - goto err_put_ddc; + return PTR_ERR(vc4_hdmi->hpd_gpio); }

vc4_hdmi->disable_wifi_frequencies = @@ -2939,7 +2949,7 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) */ ret = vc4_hdmi_runtime_resume(dev); if (ret) - goto err_put_ddc; + return ret;

pm_runtime_get_noresume(dev); pm_runtime_set_active(dev); @@ -2994,8 +3004,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) err_put_runtime_pm: pm_runtime_put_sync(dev); pm_runtime_disable(dev); -err_put_ddc: - put_device(&vc4_hdmi->ddc->dev);

return ret; } @@ -3032,8 +3040,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, vc4_hdmi_hotplug_exit(vc4_hdmi);

pm_runtime_disable(dev); - - put_device(&vc4_hdmi->ddc->dev); }

static const struct component_ops vc4_hdmi_ops = {

Commit 776efe800fed ("drm/vc4: hdmi: Drop devm interrupt handler for hotplug interrupts") dropped the device-managed interrupt registration because it was creating bugs and races whenever an interrupt was coming in while the device was removed.

However, our latest patches to the HDMI controller driver fix this as well, so we can use device-managed interrupt handlers again.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 41 +++++++++------------------------- 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 1febab3b52c6..cba51827b621 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2182,21 +2182,19 @@ static int vc4_hdmi_hotplug_init(struct vc4_hdmi *vc4_hdmi) unsigned int hpd_con = platform_get_irq_byname(pdev, "hpd-connected"); unsigned int hpd_rm = platform_get_irq_byname(pdev, "hpd-removed");

- ret = request_threaded_irq(hpd_con, - NULL, - vc4_hdmi_hpd_irq_thread, IRQF_ONESHOT, - "vc4 hdmi hpd connected", vc4_hdmi); + ret = devm_request_threaded_irq(&pdev->dev, hpd_con, + NULL, + vc4_hdmi_hpd_irq_thread, IRQF_ONESHOT, + "vc4 hdmi hpd connected", vc4_hdmi); if (ret) return ret;

- ret = request_threaded_irq(hpd_rm, - NULL, - vc4_hdmi_hpd_irq_thread, IRQF_ONESHOT, - "vc4 hdmi hpd disconnected", vc4_hdmi); - if (ret) { - free_irq(hpd_con, vc4_hdmi); + ret = devm_request_threaded_irq(&pdev->dev, hpd_rm, + NULL, + vc4_hdmi_hpd_irq_thread, IRQF_ONESHOT, + "vc4 hdmi hpd disconnected", vc4_hdmi); + if (ret) return ret; - }

connector->polled = DRM_CONNECTOR_POLL_HPD; } @@ -2204,16 +2202,6 @@ static int vc4_hdmi_hotplug_init(struct vc4_hdmi *vc4_hdmi) return 0; }

-static void vc4_hdmi_hotplug_exit(struct vc4_hdmi *vc4_hdmi) -{ - struct platform_device *pdev = vc4_hdmi->pdev; - - if (vc4_hdmi->variant->external_irq_controller) { - free_irq(platform_get_irq_byname(pdev, "hpd-connected"), vc4_hdmi); - free_irq(platform_get_irq_byname(pdev, "hpd-removed"), vc4_hdmi); - } -} - #ifdef CONFIG_DRM_VC4_HDMI_CEC static irqreturn_t vc4_cec_irq_handler_rx_thread(int irq, void *priv) { @@ -3000,11 +2988,11 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)

ret = vc4_hdmi_cec_init(vc4_hdmi); if (ret) - goto err_free_hotplug; + goto err_put_runtime_pm;

ret = vc4_hdmi_audio_init(vc4_hdmi); if (ret) - goto err_free_hotplug; + goto err_put_runtime_pm;

vc4_debugfs_add_file(drm, variant->debugfs_name, vc4_hdmi_debugfs_regs, @@ -3014,8 +3002,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)

return 0;

-err_free_hotplug: - vc4_hdmi_hotplug_exit(vc4_hdmi); err_put_runtime_pm: pm_runtime_put_sync(dev); pm_runtime_disable(dev); @@ -3026,8 +3012,6 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) static void vc4_hdmi_unbind(struct device *dev, struct device *master, void *data) { - struct vc4_hdmi *vc4_hdmi; - /* * ASoC makes it a bit hard to retrieve a pointer to the * vc4_hdmi structure. Registering the card will overwrite our @@ -3047,9 +3031,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master, */ BUILD_BUG_ON(offsetof(struct vc4_hdmi_audio, card) != 0); BUILD_BUG_ON(offsetof(struct vc4_hdmi, audio) != 0); - vc4_hdmi = dev_get_drvdata(dev); - - vc4_hdmi_hotplug_exit(vc4_hdmi);

pm_runtime_disable(dev); }

Whenever the device and driver are unbound, the main device and all the subdevices will be removed by calling their unbind() method.

However, the DRM device itself will only be freed when the last user will have closed it.

It means that there is a time window where the device and its resources aren't there anymore, but the userspace can still call into our driver.

Fortunately, the DRM framework provides the drm_dev_enter() and drm_dev_exit() functions to make sure our underlying device is still there for the section protected by those calls. Let's add them to the HDMI driver.

Acked-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_hdmi.c | 308 +++++++++++++++++++++++++++++++-- 1 file changed, 291 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 1729da01f8b1..e6678a668ff8 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -34,6 +34,7 @@ #include <drm/display/drm_hdmi_helper.h> #include <drm/display/drm_scdc_helper.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_drv.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> #include <linux/clk.h> @@ -141,17 +142,33 @@ static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused) { struct drm_info_node *node = (struct drm_info_node *)m->private; struct vc4_hdmi *vc4_hdmi = node->info_ent->data; + struct drm_device *drm = vc4_hdmi->connector.dev; struct drm_printer p = drm_seq_file_printer(m); + int idx; + + if (!drm_dev_enter(drm, &idx)) + return -ENODEV;

drm_print_regset32(&p, &vc4_hdmi->hdmi_regset); drm_print_regset32(&p, &vc4_hdmi->hd_regset);

+ drm_dev_exit(idx); + return 0; }

static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi) { + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx; + + /* + * We can be called by our bind callback, when the + * connector->dev pointer might not be initialised yet. + */ + if (drm && !drm_dev_enter(drm, &idx)) + return;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -168,11 +185,23 @@ static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi) HDMI_WRITE(HDMI_SW_RESET_CONTROL, 0);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + if (drm) + drm_dev_exit(idx); }

static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi) { + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx; + + /* + * We can be called by our bind callback, when the + * connector->dev pointer might not be initialised yet. + */ + if (drm && !drm_dev_enter(drm, &idx)) + return;

reset_control_reset(vc4_hdmi->reset);

@@ -184,15 +213,31 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi) HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + if (drm) + drm_dev_exit(idx); }

#ifdef CONFIG_DRM_VC4_HDMI_CEC static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) { - unsigned long cec_rate = clk_get_rate(vc4_hdmi->cec_clock); + struct drm_device *drm = vc4_hdmi->connector.dev; + unsigned long cec_rate; unsigned long flags; u16 clk_cnt; u32 value; + int idx; + + /* + * This function is called by our runtime_resume implementation + * and thus at bind time, when we haven't registered our + * connector yet and thus don't have a pointer to the DRM + * device. + */ + if (drm && !drm_dev_enter(drm, &idx)) + return; + + cec_rate = clk_get_rate(vc4_hdmi->cec_clock);

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -208,6 +253,9 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) HDMI_WRITE(HDMI_CEC_CNTRL_1, value);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + if (drm) + drm_dev_exit(idx); } #else static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {} @@ -428,25 +476,34 @@ static int vc4_hdmi_stop_packet(struct drm_encoder *encoder, bool poll) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; u32 packet_id = type - 0x80; unsigned long flags; + int ret = 0; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return -ENODEV;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, HDMI_READ(HDMI_RAM_PACKET_CONFIG) & ~BIT(packet_id)); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

- if (!poll) - return 0; + if (poll) { + ret = wait_for(!(HDMI_READ(HDMI_RAM_PACKET_STATUS) & + BIT(packet_id)), 100); + }

- return wait_for(!(HDMI_READ(HDMI_RAM_PACKET_STATUS) & - BIT(packet_id)), 100); + drm_dev_exit(idx); + return ret; }

static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder, union hdmi_infoframe *frame) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; u32 packet_id = frame->any.type - 0x80; const struct vc4_hdmi_register *ram_packet_start = &vc4_hdmi->variant->registers[HDMI_RAM_PACKET_START]; @@ -457,6 +514,10 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder, unsigned long flags; ssize_t len, i; int ret; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

WARN_ONCE(!(HDMI_READ(HDMI_RAM_PACKET_CONFIG) & VC4_HDMI_RAM_PACKET_ENABLE), @@ -464,12 +525,12 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder,

len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer)); if (len < 0) - return; + goto out;

ret = vc4_hdmi_stop_packet(encoder, frame->any.type, true); if (ret) { DRM_ERROR("Failed to wait for infoframe to go idle: %d\n", ret); - return; + goto out; }

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); @@ -498,6 +559,9 @@ static void vc4_hdmi_write_infoframe(struct drm_encoder *encoder, BIT(packet_id)), 100); if (ret) DRM_ERROR("Failed to wait for infoframe to start: %d\n", ret); + +out: + drm_dev_exit(idx); }

static void vc4_hdmi_avi_infoframe_colorspace(struct hdmi_avi_infoframe *frame, @@ -645,8 +709,10 @@ static bool vc4_hdmi_supports_scrambling(struct drm_encoder *encoder, static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; unsigned long flags; + int idx;

lockdep_assert_held(&vc4_hdmi->mutex);

@@ -658,6 +724,9 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) vc4_hdmi->output_format)) return;

+ if (!drm_dev_enter(drm, &idx)) + return; + drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, true); drm_scdc_set_scrambling(vc4_hdmi->ddc, true);

@@ -666,6 +735,8 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) VC5_HDMI_SCRAMBLER_CTL_ENABLE); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

+ drm_dev_exit(idx); + vc4_hdmi->scdc_enabled = true;

queue_delayed_work(system_wq, &vc4_hdmi->scrambling_work, @@ -675,7 +746,9 @@ static void vc4_hdmi_enable_scrambling(struct drm_encoder *encoder) static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx;

lockdep_assert_held(&vc4_hdmi->mutex);

@@ -687,6 +760,9 @@ static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder) if (delayed_work_pending(&vc4_hdmi->scrambling_work)) cancel_delayed_work_sync(&vc4_hdmi->scrambling_work);

+ if (!drm_dev_enter(drm, &idx)) + return; + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_SCRAMBLER_CTL, HDMI_READ(HDMI_SCRAMBLER_CTL) & ~VC5_HDMI_SCRAMBLER_CTL_ENABLE); @@ -694,6 +770,8 @@ static void vc4_hdmi_disable_scrambling(struct drm_encoder *encoder)

drm_scdc_set_scrambling(vc4_hdmi->ddc, false); drm_scdc_set_high_tmds_clock_ratio(vc4_hdmi->ddc, false); + + drm_dev_exit(idx); }

static void vc4_hdmi_scrambling_wq(struct work_struct *work) @@ -716,10 +794,15 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) + goto out; + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

HDMI_WRITE(HDMI_RAM_PACKET_CONFIG, 0); @@ -737,6 +820,9 @@ static void vc4_hdmi_encoder_post_crtc_disable(struct drm_encoder *encoder,

vc4_hdmi_disable_scrambling(encoder);

+ drm_dev_exit(idx); + +out: mutex_unlock(&vc4_hdmi->mutex); }

@@ -744,11 +830,16 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; int ret; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) + goto out; + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_VID_CTL, HDMI_READ(HDMI_VID_CTL) | VC4_HD_VID_CTL_BLANKPIX); @@ -764,6 +855,9 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder, if (ret < 0) DRM_ERROR("Failed to release power domain: %d\n", ret);

+ drm_dev_exit(idx); + +out: mutex_unlock(&vc4_hdmi->mutex); }

@@ -780,8 +874,13 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, struct drm_connector_state *state, const struct drm_display_mode *mode) { + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; u32 csc_ctl; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -816,6 +915,8 @@ static void vc4_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, HDMI_WRITE(HDMI_CSC_CTL, csc_ctl);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + drm_dev_exit(idx); }

/* @@ -900,6 +1001,7 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, struct drm_connector_state *state, const struct drm_display_mode *mode) { + struct drm_device *drm = vc4_hdmi->connector.dev; struct vc4_hdmi_connector_state *vc4_state = conn_state_to_vc4_hdmi_conn_state(state); unsigned long flags; @@ -908,6 +1010,10 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, u32 csc_chan_ctl = 0; u32 csc_ctl = VC5_MT_CP_CSC_CTL_ENABLE | VC4_SET_FIELD(VC4_HD_CSC_CTL_MODE_CUSTOM, VC5_MT_CP_CSC_CTL_MODE); + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -950,12 +1056,15 @@ static void vc5_hdmi_csc_setup(struct vc4_hdmi *vc4_hdmi, HDMI_WRITE(HDMI_CSC_CTL, csc_ctl);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + drm_dev_exit(idx); }

static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, struct drm_connector_state *state, struct drm_display_mode *mode) { + struct drm_device *drm = vc4_hdmi->connector.dev; bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC; bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC; bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE; @@ -974,6 +1083,10 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, interlaced, VC4_HDMI_VERTB_VBP)); unsigned long flags; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -1001,12 +1114,15 @@ static void vc4_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, HDMI_WRITE(HDMI_VERTB1, vertb);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + drm_dev_exit(idx); }

static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, struct drm_connector_state *state, struct drm_display_mode *mode) { + struct drm_device *drm = vc4_hdmi->connector.dev; const struct vc4_hdmi_connector_state *vc4_state = conn_state_to_vc4_hdmi_conn_state(state); bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC; @@ -1030,6 +1146,10 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, unsigned char gcp; bool gcp_en; u32 reg; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -1101,13 +1221,20 @@ static void vc5_hdmi_set_timings(struct vc4_hdmi *vc4_hdmi, HDMI_WRITE(HDMI_CLOCK_STOP, 0);

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + drm_dev_exit(idx); }

static void vc4_hdmi_recenter_fifo(struct vc4_hdmi *vc4_hdmi) { + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; u32 drift; int ret; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -1136,12 +1263,15 @@ static void vc4_hdmi_recenter_fifo(struct vc4_hdmi *vc4_hdmi) VC4_HDMI_FIFO_CTL_RECENTER_DONE, 1); WARN_ONCE(ret, "Timeout waiting for " "VC4_HDMI_FIFO_CTL_RECENTER_DONE"); + + drm_dev_exit(idx); }

static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; struct drm_connector *connector = &vc4_hdmi->connector; struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state, connector); @@ -1152,9 +1282,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, unsigned long bvb_rate, hsm_rate; unsigned long flags; int ret; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) + goto out; + /* * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must * be faster than pixel clock, infinitesimally faster, tested in @@ -1175,13 +1309,13 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate); if (ret) { DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); - goto out; + goto err_dev_exit; }

ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); if (ret < 0) { DRM_ERROR("Failed to retain power domain: %d\n", ret); - goto out; + goto err_dev_exit; }

ret = clk_set_rate(vc4_hdmi->pixel_clock, tmds_char_rate); @@ -1233,6 +1367,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, if (vc4_hdmi->variant->set_timings) vc4_hdmi->variant->set_timings(vc4_hdmi, conn_state, mode);

+ drm_dev_exit(idx); + mutex_unlock(&vc4_hdmi->mutex);

return; @@ -1241,6 +1377,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, clk_disable_unprepare(vc4_hdmi->pixel_clock); err_put_runtime_pm: pm_runtime_put(&vc4_hdmi->pdev->dev); +err_dev_exit: + drm_dev_exit(idx); out: mutex_unlock(&vc4_hdmi->mutex); return; @@ -1250,14 +1388,19 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; struct drm_connector *connector = &vc4_hdmi->connector; struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state, connector); unsigned long flags; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) + return; + if (vc4_hdmi->variant->csc_setup) vc4_hdmi->variant->csc_setup(vc4_hdmi, conn_state, mode);

@@ -1265,6 +1408,8 @@ static void vc4_hdmi_encoder_pre_crtc_enable(struct drm_encoder *encoder, HDMI_WRITE(HDMI_FIFO_CTL, VC4_HDMI_FIFO_CTL_MASTER_SLAVE_N); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

+ drm_dev_exit(idx); + mutex_unlock(&vc4_hdmi->mutex); }

@@ -1272,15 +1417,20 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, struct drm_atomic_state *state) { struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder); + struct drm_device *drm = vc4_hdmi->connector.dev; struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; struct drm_display_info *display = &vc4_hdmi->connector.display_info; bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC; bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC; unsigned long flags; int ret; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) + return; + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

HDMI_WRITE(HDMI_VID_CTL, @@ -1341,6 +1491,7 @@ static void vc4_hdmi_encoder_post_crtc_enable(struct drm_encoder *encoder, vc4_hdmi_recenter_fifo(vc4_hdmi); vc4_hdmi_enable_scrambling(encoder);

+ drm_dev_exit(idx); mutex_unlock(&vc4_hdmi->mutex); }

@@ -1676,13 +1827,20 @@ static u32 vc5_hdmi_channel_map(struct vc4_hdmi *vc4_hdmi, u32 channel_mask)

static bool vc5_hdmi_hp_detect(struct vc4_hdmi *vc4_hdmi) { + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; u32 hotplug; + int idx; + + if (!drm_dev_enter(drm, &idx)) + return false;

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); hotplug = HDMI_READ(HDMI_HOTPLUG); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

+ drm_dev_exit(idx); + return !!(hotplug & VC4_HDMI_HOTPLUG_CONNECTED); }

@@ -1690,10 +1848,16 @@ static bool vc5_hdmi_hp_detect(struct vc4_hdmi *vc4_hdmi) static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) { - u32 hsm_clock = clk_get_rate(vc4_hdmi->audio_clock); + struct drm_device *drm = vc4_hdmi->connector.dev; + u32 hsm_clock; unsigned long flags; unsigned long n, m; + int idx;

+ if (!drm_dev_enter(drm, &idx)) + return; + + hsm_clock = clk_get_rate(vc4_hdmi->audio_clock); rational_best_approximation(hsm_clock, samplerate, VC4_HD_MAI_SMP_N_MASK >> VC4_HD_MAI_SMP_N_SHIFT, @@ -1706,6 +1870,8 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi, VC4_SET_FIELD(n, VC4_HD_MAI_SMP_N) | VC4_SET_FIELD(m - 1, VC4_HD_MAI_SMP_M)); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags); + + drm_dev_exit(idx); }

static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate) @@ -1765,13 +1931,21 @@ static bool vc4_hdmi_audio_can_stream(struct vc4_hdmi *vc4_hdmi) static int vc4_hdmi_audio_startup(struct device *dev, void *data) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int ret = 0; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) { + ret = -ENODEV; + goto out; + } + if (!vc4_hdmi_audio_can_stream(vc4_hdmi)) { - mutex_unlock(&vc4_hdmi->mutex); - return -ENODEV; + ret = -ENODEV; + goto out_dev_exit; }

vc4_hdmi->audio.streaming = true; @@ -1788,9 +1962,12 @@ static int vc4_hdmi_audio_startup(struct device *dev, void *data) if (vc4_hdmi->variant->phy_rng_enable) vc4_hdmi->variant->phy_rng_enable(vc4_hdmi);

+out_dev_exit: + drm_dev_exit(idx); +out: mutex_unlock(&vc4_hdmi->mutex);

- return 0; + return ret; }

static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi) @@ -1819,10 +1996,15 @@ static void vc4_hdmi_audio_reset(struct vc4_hdmi *vc4_hdmi) static void vc4_hdmi_audio_shutdown(struct device *dev, void *data) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx;

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) + goto out; + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

HDMI_WRITE(HDMI_MAI_CTL, @@ -1838,6 +2020,9 @@ static void vc4_hdmi_audio_shutdown(struct device *dev, void *data) vc4_hdmi->audio.streaming = false; vc4_hdmi_audio_reset(vc4_hdmi);

+ drm_dev_exit(idx); + +out: mutex_unlock(&vc4_hdmi->mutex); }

@@ -1885,6 +2070,7 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, struct hdmi_codec_params *params) { struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev); + struct drm_device *drm = vc4_hdmi->connector.dev; struct drm_encoder *encoder = &vc4_hdmi->encoder.base; unsigned int sample_rate = params->sample_rate; unsigned int channels = params->channels; @@ -1893,15 +2079,22 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, u32 channel_map; u32 mai_audio_format; u32 mai_sample_rate; + int ret = 0; + int idx;

dev_dbg(dev, "%s: %u Hz, %d bit, %d channels\n", __func__, sample_rate, params->sample_width, channels);

mutex_lock(&vc4_hdmi->mutex);

+ if (!drm_dev_enter(drm, &idx)) { + ret = -ENODEV; + goto out; + } + if (!vc4_hdmi_audio_can_stream(vc4_hdmi)) { - mutex_unlock(&vc4_hdmi->mutex); - return -EINVAL; + ret = -EINVAL; + goto out_dev_exit; }

vc4_hdmi_audio_set_mai_clock(vc4_hdmi, sample_rate); @@ -1958,9 +2151,12 @@ static int vc4_hdmi_audio_prepare(struct device *dev, void *data, memcpy(&vc4_hdmi->audio.infoframe, &params->cea, sizeof(params->cea)); vc4_hdmi_set_audio_infoframe(encoder);

+out_dev_exit: + drm_dev_exit(idx); +out: mutex_unlock(&vc4_hdmi->mutex);

- return 0; + return ret; }

static const struct snd_soc_component_driver vc4_hdmi_audio_cpu_dai_comp = { @@ -2295,6 +2491,17 @@ static irqreturn_t vc4_cec_irq_handler_tx_bare_locked(struct vc4_hdmi *vc4_hdmi) { u32 cntrl1;

+ /* + * We don't need to protect the register access using + * drm_dev_enter() there because the interrupt handler lifetime + * is tied to the device itself, and not to the DRM device. + * + * So when the device will be gone, one of the first thing we + * will be doing will be to unregister the interrupt handler, + * and then unregister the DRM device. drm_dev_enter() would + * thus always succeed if we are here. + */ + lockdep_assert_held(&vc4_hdmi->hw_lock);

cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); @@ -2323,6 +2530,17 @@ static irqreturn_t vc4_cec_irq_handler_rx_bare_locked(struct vc4_hdmi *vc4_hdmi)

lockdep_assert_held(&vc4_hdmi->hw_lock);

+ /* + * We don't need to protect the register access using + * drm_dev_enter() there because the interrupt handler lifetime + * is tied to the device itself, and not to the DRM device. + * + * So when the device will be gone, one of the first thing we + * will be doing will be to unregister the interrupt handler, + * and then unregister the DRM device. drm_dev_enter() would + * thus always succeed if we are here. + */ + vc4_hdmi->cec_rx_msg.len = 0; cntrl1 = HDMI_READ(HDMI_CEC_CNTRL_1); vc4_cec_read_msg(vc4_hdmi, cntrl1); @@ -2354,6 +2572,17 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv) irqreturn_t ret; u32 cntrl5;

+ /* + * We don't need to protect the register access using + * drm_dev_enter() there because the interrupt handler lifetime + * is tied to the device itself, and not to the DRM device. + * + * So when the device will be gone, one of the first thing we + * will be doing will be to unregister the interrupt handler, + * and then unregister the DRM device. drm_dev_enter() would + * thus always succeed if we are here. + */ + if (!(stat & VC4_HDMI_CPU_CEC)) return IRQ_NONE;

@@ -2374,11 +2603,13 @@ static irqreturn_t vc4_cec_irq_handler(int irq, void *priv) static int vc4_hdmi_cec_enable(struct cec_adapter *adap) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + struct drm_device *drm = vc4_hdmi->connector.dev; /* clock period in microseconds */ const u32 usecs = 1000000 / CEC_CLOCK_FREQ; unsigned long flags; u32 val; int ret; + int idx;

/* * NOTE: This function should really take vc4_hdmi->mutex, but doing so @@ -2391,9 +2622,19 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap) * keep it in mind if we were to change that assumption. */

+ if (!drm_dev_enter(drm, &idx)) + /* + * We can't return an error code, because the CEC + * framework will emit WARN_ON messages at unbind + * otherwise. + */ + return 0; + ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); - if (ret) + if (ret) { + drm_dev_exit(idx); return ret; + }

spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);

@@ -2429,13 +2670,25 @@ static int vc4_hdmi_cec_enable(struct cec_adapter *adap)

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

+ drm_dev_exit(idx); + return 0; }

static int vc4_hdmi_cec_disable(struct cec_adapter *adap) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx; + + if (!drm_dev_enter(drm, &idx)) + /* + * We can't return an error code, because the CEC + * framework will emit WARN_ON messages at unbind + * otherwise. + */ + return 0;

/* * NOTE: This function should really take vc4_hdmi->mutex, but doing so @@ -2460,6 +2713,8 @@ static int vc4_hdmi_cec_disable(struct cec_adapter *adap)

pm_runtime_put(&vc4_hdmi->pdev->dev);

+ drm_dev_exit(idx); + return 0; }

@@ -2474,7 +2729,9 @@ static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable) static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr) { struct vc4_hdmi *vc4_hdmi = cec_get_drvdata(adap); + struct drm_device *drm = vc4_hdmi->connector.dev; unsigned long flags; + int idx;

/* * NOTE: This function should really take vc4_hdmi->mutex, but doing so @@ -2487,12 +2744,22 @@ static int vc4_hdmi_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr) * keep it in mind if we were to change that assumption. */

+ if (!drm_dev_enter(drm, &idx)) + /* + * We can't return an error code, because the CEC + * framework will emit WARN_ON messages at unbind + * otherwise. + */ + return 0; + spin_lock_irqsave(&vc4_hdmi->hw_lock, flags); HDMI_WRITE(HDMI_CEC_CNTRL_1, (HDMI_READ(HDMI_CEC_CNTRL_1) & ~VC4_HDMI_CEC_ADDR_MASK) | (log_addr & 0xf) << VC4_HDMI_CEC_ADDR_SHIFT); spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

+ drm_dev_exit(idx); + return 0; }

@@ -2504,6 +2771,7 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, unsigned long flags; u32 val; unsigned int i; + int idx;

/* * NOTE: This function should really take vc4_hdmi->mutex, but doing so @@ -2516,8 +2784,12 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts, * keep it in mind if we were to change that assumption. */

+ if (!drm_dev_enter(dev, &idx)) + return -ENODEV; + if (msg->len > 16) { drm_err(dev, "Attempting to transmit too much data (%d)\n", msg->len); + drm_dev_exit(idx); return -ENOMEM; }

@@ -2541,6 +2813,8 @@ static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,

spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);

+ drm_dev_exit(idx); + return 0; }

There's no user for that pointer so let's just get rid of it.

Acked-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_drv.h | 1 - drivers/gpu/drm/vc4/vc4_txp.c | 6 ------ 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index db51dd3e20b8..9c5b31fa4b9c 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -84,7 +84,6 @@ struct vc4_dev { struct vc4_hvs *hvs; struct vc4_v3d *v3d; struct vc4_vec *vec; - struct vc4_txp *txp;

struct vc4_hang_state *hang_state;

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 949db41c50fb..0e868aebc25f 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -468,7 +468,6 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) { struct platform_device *pdev = to_platform_device(dev); struct drm_device *drm = dev_get_drvdata(master); - struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_crtc *vc4_crtc; struct vc4_txp *txp; struct drm_crtc *crtc; @@ -522,7 +521,6 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) return ret;

dev_set_drvdata(dev, txp); - vc4->txp = txp;

vc4_debugfs_add_regset32(drm, "txp_regs", &txp->regset);

@@ -532,13 +530,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) static void vc4_txp_unbind(struct device *dev, struct device *master, void *data) { - struct drm_device *drm = dev_get_drvdata(master); - struct vc4_dev *vc4 = to_vc4_dev(drm); struct vc4_txp *txp = dev_get_drvdata(dev);

vc4_txp_connector_destroy(&txp->connector.base); - - vc4->txp = NULL; }

static const struct component_ops vc4_txp_ops = {

Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc.

This means that this will eventually be freed whenever the device is removed. In our case, the most likely source of removal is that the main device is going to be unbound, and component_unbind_all() is being run.

However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free.

Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore.

Acked-by: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_txp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 10862a850d6e..93576b2d9ba6 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -477,7 +477,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) if (irq < 0) return irq;

- txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL); + txp = drmm_kzalloc(drm, sizeof(*txp), GFP_KERNEL); if (!txp) return -ENOMEM; vc4_crtc = &txp->base;

Our current code now mixes some resources whose lifetime are tied to the device (clocks, IO mappings, etc.) and some that are tied to the DRM device (encoder, bridge).

The device one will be freed at unbind time, but the DRM one will only be freed when the last user of the DRM device closes its file handle.

So we end up with a time window during which we can call the encoder hooks, but we don't have access to the underlying resources and device.

Let's protect all those sections with drm_dev_enter() and drm_dev_exit() so that we bail out if we are during that window.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_txp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 0976d29adc7f..0748e3f6d40f 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -15,6 +15,7 @@

#include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> +#include <drm/drm_drv.h> #include <drm/drm_edid.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fourcc.h> @@ -274,6 +275,7 @@ static int vc4_txp_connector_atomic_check(struct drm_connector *conn, static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, struct drm_atomic_state *state) { + struct drm_device *drm = conn->dev; struct drm_connector_state *conn_state = drm_atomic_get_new_connector_state(state, conn); struct vc4_txp *txp = connector_to_vc4_txp(conn); @@ -281,6 +283,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, struct drm_display_mode *mode; struct drm_framebuffer *fb; u32 ctrl; + int idx; int i;

if (WARN_ON(!conn_state->writeback_job)) @@ -310,6 +313,9 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, */ ctrl |= TXP_ALPHA_INVERT;

+ if (!drm_dev_enter(drm, &idx)) + return; + gem = drm_fb_cma_get_gem_obj(fb, 0); TXP_WRITE(TXP_DST_PTR, gem->paddr + fb->offsets[0]); TXP_WRITE(TXP_DST_PITCH, fb->pitches[0]); @@ -320,6 +326,8 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, TXP_WRITE(TXP_DST_CTRL, ctrl);

drm_writeback_queue_job(&txp->connector, conn_state); + + drm_dev_exit(idx); }

static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = { @@ -346,7 +354,12 @@ static const struct drm_connector_funcs vc4_txp_connector_funcs = {

static void vc4_txp_encoder_disable(struct drm_encoder *encoder) { + struct drm_device *drm = encoder->dev; struct vc4_txp *txp = encoder_to_vc4_txp(encoder); + int idx; + + if (!drm_dev_enter(drm, &idx)) + return;

if (TXP_READ(TXP_DST_CTRL) & TXP_BUSY) { unsigned long timeout = jiffies + msecs_to_jiffies(1000); @@ -361,6 +374,8 @@ static void vc4_txp_encoder_disable(struct drm_encoder *encoder) }

TXP_WRITE(TXP_DST_CTRL, TXP_POWERDOWN); + + drm_dev_exit(idx); }

static const struct drm_encoder_helper_funcs vc4_txp_encoder_helper_funcs = { @@ -444,6 +459,16 @@ static irqreturn_t vc4_txp_interrupt(int irq, void *data) struct vc4_txp *txp = data; struct vc4_crtc *vc4_crtc = &txp->base;

+ /* + * We don't need to protect the register access using + * drm_dev_enter() there because the interrupt handler lifetime + * is tied to the device itself, and not to the DRM device. + * + * So when the device will be gone, one of the first thing we + * will be doing will be to unregister the interrupt handler, + * and then unregister the DRM device. drm_dev_enter() would + * thus always succeed if we are here. + */ TXP_WRITE(TXP_DST_CTRL, TXP_READ(TXP_DST_CTRL) & ~TXP_EI); vc4_crtc_handle_vblank(vc4_crtc); drm_writeback_signal_completion(&txp->connector, 0);

The VC4 VEC driver private structure contains only a pointer to the encoder and connector it implements. This makes the overall structure somewhat inconsistent with the rest of the driver, and complicates its initialisation without any apparent gain.

Let's embed the drm_encoder structure (through the vc4_encoder one) and drm_connector into struct vc4_vec to fix both issues.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_vec.c | 83 +++++++++-------------------------- 1 file changed, 21 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index 99fe40c8cf81..2c96d5adcf49 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -160,12 +160,12 @@ struct vc4_vec_variant {

/* General VEC hardware state. */ struct vc4_vec { + struct vc4_encoder encoder; + struct drm_connector connector; + struct platform_device *pdev; const struct vc4_vec_variant *variant;

- struct drm_encoder *encoder; - struct drm_connector *connector; - void __iomem *regs;

struct clk *clock; @@ -178,30 +178,12 @@ struct vc4_vec { #define VEC_READ(offset) readl(vec->regs + (offset)) #define VEC_WRITE(offset, val) writel(val, vec->regs + (offset))

-/* VC4 VEC encoder KMS struct */ -struct vc4_vec_encoder { - struct vc4_encoder base; - struct vc4_vec *vec; -}; - -static inline struct vc4_vec_encoder * -to_vc4_vec_encoder(struct drm_encoder *encoder) +static inline struct vc4_vec * +encoder_to_vc4_vec(struct drm_encoder *encoder) { - return container_of(encoder, struct vc4_vec_encoder, base.base); + return container_of(encoder, struct vc4_vec, encoder.base); }

-/* VC4 VEC connector KMS struct */ -struct vc4_vec_connector { - struct drm_connector base; - struct vc4_vec *vec; - - /* Since the connector is attached to just the one encoder, - * this is the reference to it so we can do the best_encoder() - * hook. - */ - struct drm_encoder *encoder; -}; - enum vc4_vec_tv_mode_id { VC4_VEC_TV_MODE_NTSC, VC4_VEC_TV_MODE_NTSC_J, @@ -343,22 +325,12 @@ static const struct drm_connector_helper_funcs vc4_vec_connector_helper_funcs = .get_modes = vc4_vec_connector_get_modes, };

-static struct drm_connector *vc4_vec_connector_init(struct drm_device *dev, - struct vc4_vec *vec) +static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec) { - struct drm_connector *connector = NULL; - struct vc4_vec_connector *vec_connector; + struct drm_connector *connector = &vec->connector;

- vec_connector = devm_kzalloc(dev->dev, sizeof(*vec_connector), - GFP_KERNEL); - if (!vec_connector) - return ERR_PTR(-ENOMEM); - - connector = &vec_connector->base; connector->interlace_allowed = true;

- vec_connector->encoder = vec->encoder; - vec_connector->vec = vec;

drm_connector_init(dev, connector, &vc4_vec_connector_funcs, DRM_MODE_CONNECTOR_Composite); @@ -369,15 +341,14 @@ static struct drm_connector *vc4_vec_connector_init(struct drm_device *dev, VC4_VEC_TV_MODE_NTSC); vec->tv_mode = &vc4_vec_tv_modes[VC4_VEC_TV_MODE_NTSC];

- drm_connector_attach_encoder(connector, vec->encoder); + drm_connector_attach_encoder(connector, &vec->encoder.base);

- return connector; + return 0; }

static void vc4_vec_encoder_disable(struct drm_encoder *encoder) { - struct vc4_vec_encoder *vc4_vec_encoder = to_vc4_vec_encoder(encoder); - struct vc4_vec *vec = vc4_vec_encoder->vec; + struct vc4_vec *vec = encoder_to_vc4_vec(encoder); int ret;

VEC_WRITE(VEC_CFG, 0); @@ -398,8 +369,7 @@ static void vc4_vec_encoder_disable(struct drm_encoder *encoder)

static void vc4_vec_encoder_enable(struct drm_encoder *encoder) { - struct vc4_vec_encoder *vc4_vec_encoder = to_vc4_vec_encoder(encoder); - struct vc4_vec *vec = vc4_vec_encoder->vec; + struct vc4_vec *vec = encoder_to_vc4_vec(encoder); int ret;

ret = pm_runtime_get_sync(&vec->pdev->dev); @@ -474,8 +444,7 @@ static void vc4_vec_encoder_atomic_mode_set(struct drm_encoder *encoder, struct drm_crtc_state *crtc_state, struct drm_connector_state *conn_state) { - struct vc4_vec_encoder *vc4_vec_encoder = to_vc4_vec_encoder(encoder); - struct vc4_vec *vec = vc4_vec_encoder->vec; + struct vc4_vec *vec = encoder_to_vc4_vec(encoder);

vec->tv_mode = &vc4_vec_tv_modes[conn_state->tv.mode]; } @@ -533,7 +502,6 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) struct platform_device *pdev = to_platform_device(dev); struct drm_device *drm = dev_get_drvdata(master); struct vc4_vec *vec; - struct vc4_vec_encoder *vc4_vec_encoder; int ret;

ret = drm_mode_create_tv_properties(drm, ARRAY_SIZE(tv_mode_names), @@ -545,14 +513,7 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) if (!vec) return -ENOMEM;

- vc4_vec_encoder = devm_kzalloc(dev, sizeof(*vc4_vec_encoder), - GFP_KERNEL); - if (!vc4_vec_encoder) - return -ENOMEM; - vc4_vec_encoder->base.type = VC4_ENCODER_TYPE_VEC; - vc4_vec_encoder->vec = vec; - vec->encoder = &vc4_vec_encoder->base.base; - + vec->encoder.type = VC4_ENCODER_TYPE_VEC; vec->pdev = pdev; vec->variant = (const struct vc4_vec_variant *) of_device_get_match_data(dev); @@ -573,14 +534,12 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data)

pm_runtime_enable(dev);

- drm_simple_encoder_init(drm, vec->encoder, DRM_MODE_ENCODER_TVDAC); - drm_encoder_helper_add(vec->encoder, &vc4_vec_encoder_helper_funcs); + drm_simple_encoder_init(drm, &vec->encoder.base, DRM_MODE_ENCODER_TVDAC); + drm_encoder_helper_add(&vec->encoder.base, &vc4_vec_encoder_helper_funcs);

- vec->connector = vc4_vec_connector_init(drm, vec); - if (IS_ERR(vec->connector)) { - ret = PTR_ERR(vec->connector); + ret = vc4_vec_connector_init(drm, vec); + if (ret) goto err_destroy_encoder; - }

dev_set_drvdata(dev, vec);

@@ -589,7 +548,7 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) return 0;

err_destroy_encoder: - drm_encoder_cleanup(vec->encoder); + drm_encoder_cleanup(&vec->encoder.base); pm_runtime_disable(dev);

return ret; @@ -600,8 +559,8 @@ static void vc4_vec_unbind(struct device *dev, struct device *master, { struct vc4_vec *vec = dev_get_drvdata(dev);

- vc4_vec_connector_destroy(vec->connector); - drm_encoder_cleanup(vec->encoder); + vc4_vec_connector_destroy(&vec->connector); + drm_encoder_cleanup(&vec->encoder.base); pm_runtime_disable(dev); }

drm_connector_unregister() is only to be used for connectors that have been registered through drm_connector_register() after drm_dev_register() has been called. This is our case here so let's remove the call.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_vec.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index a051b25337c0..2e9dc09fc6f0 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -289,12 +289,6 @@ vc4_vec_connector_detect(struct drm_connector *connector, bool force) return connector_status_unknown; }

-static void vc4_vec_connector_destroy(struct drm_connector *connector) -{ - drm_connector_unregister(connector); - drm_connector_cleanup(connector); -} - static int vc4_vec_connector_get_modes(struct drm_connector *connector) { struct drm_connector_state *state = connector->state; @@ -315,7 +309,7 @@ static int vc4_vec_connector_get_modes(struct drm_connector *connector) static const struct drm_connector_funcs vc4_vec_connector_funcs = { .detect = vc4_vec_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = vc4_vec_connector_destroy, + .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, @@ -559,7 +553,7 @@ static void vc4_vec_unbind(struct device *dev, struct device *master, { struct vc4_vec *vec = dev_get_drvdata(dev);

- vc4_vec_connector_destroy(&vec->connector); + drm_connector_cleanup(&vec->connector); drm_encoder_cleanup(&vec->encoder.base); pm_runtime_disable(dev); }

The current code will call drm_connector_unregister() and drm_connector_cleanup() when the device is unbound. However, by then, there might still be some references held to that connector, including by the userspace that might still have the DRM device open.

Let's switch to a DRM-managed initialization to clean up after ourselves only once the DRM device has been last closed.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_vec.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index 518b66bcc0b5..f6858e40ddbc 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -309,7 +309,6 @@ static int vc4_vec_connector_get_modes(struct drm_connector *connector) static const struct drm_connector_funcs vc4_vec_connector_funcs = { .detect = vc4_vec_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, - .destroy = drm_connector_cleanup, .reset = drm_atomic_helper_connector_reset, .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, @@ -322,12 +321,15 @@ static const struct drm_connector_helper_funcs vc4_vec_connector_helper_funcs = static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec *vec) { struct drm_connector *connector = &vec->connector; + int ret;

connector->interlace_allowed = true;

+ ret = drmm_connector_init(dev, connector, &vc4_vec_connector_funcs, + DRM_MODE_CONNECTOR_Composite); + if (ret) + return ret;

- drm_connector_init(dev, connector, &vc4_vec_connector_funcs, - DRM_MODE_CONNECTOR_Composite); drm_connector_helper_add(connector, &vc4_vec_connector_helper_funcs);

drm_object_attach_property(&connector->base, @@ -556,9 +558,6 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) static void vc4_vec_unbind(struct device *dev, struct device *master, void *data) { - struct vc4_vec *vec = dev_get_drvdata(dev); - - drm_connector_cleanup(&vec->connector); pm_runtime_disable(dev); }

devm_pm_runtime_enable() simplifies the driver a bit since it will call pm_runtime_disable() automatically through a device-managed action.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_vec.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c index b2a15c39939e..7e02fbad3208 100644 --- a/drivers/gpu/drm/vc4/vc4_vec.c +++ b/drivers/gpu/drm/vc4/vc4_vec.c @@ -583,42 +583,32 @@ static int vc4_vec_bind(struct device *dev, struct device *master, void *data) return ret; }

- pm_runtime_enable(dev); + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret;

ret = drmm_encoder_init(drm, &vec->encoder.base, NULL, DRM_MODE_ENCODER_TVDAC, NULL); if (ret) - goto err_put_runtime_pm; + return ret;

drm_encoder_helper_add(&vec->encoder.base, &vc4_vec_encoder_helper_funcs);

ret = vc4_vec_connector_init(drm, vec); if (ret) - goto err_put_runtime_pm; + return ret;

dev_set_drvdata(dev, vec);

vc4_debugfs_add_regset32(drm, "vec_regs", &vec->regset);

return 0; - -err_put_runtime_pm: - pm_runtime_disable(dev); - - return ret; -} - -static void vc4_vec_unbind(struct device *dev, struct device *master, - void *data) -{ - pm_runtime_disable(dev); }

static const struct component_ops vc4_vec_ops = { .bind = vc4_vec_bind, - .unbind = vc4_vec_unbind, };

static int vc4_vec_dev_probe(struct platform_device *pdev)

vc4_debugfs_add_file() can fail, so let's propagate its error code instead of silencing it.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_debugfs.c | 20 +++++++++++--------- drivers/gpu/drm/vc4/vc4_drv.h | 30 ++++++++++++++++-------------- 2 files changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_debugfs.c b/drivers/gpu/drm/vc4/vc4_debugfs.c index d6350a8ca048..b857fb9c94bc 100644 --- a/drivers/gpu/drm/vc4/vc4_debugfs.c +++ b/drivers/gpu/drm/vc4/vc4_debugfs.c @@ -67,10 +67,10 @@ static int vc4_debugfs_regset32(struct seq_file *m, void *unused) * track the request and delay it to be called on each minor during * vc4_debugfs_init(). */ -void vc4_debugfs_add_file(struct drm_device *dev, - const char *name, - int (*show)(struct seq_file*, void*), - void *data) +int vc4_debugfs_add_file(struct drm_device *dev, + const char *name, + int (*show)(struct seq_file*, void*), + void *data) { struct vc4_dev *vc4 = to_vc4_dev(dev);

@@ -78,18 +78,20 @@ void vc4_debugfs_add_file(struct drm_device *dev, devm_kzalloc(dev->dev, sizeof(*entry), GFP_KERNEL);

if (!entry) - return; + return -ENOMEM;

entry->info.name = name; entry->info.show = show; entry->info.data = data;

list_add(&entry->link, &vc4->debugfs_list); + + return 0; }

-void vc4_debugfs_add_regset32(struct drm_device *drm, - const char *name, - struct debugfs_regset32 *regset) +int vc4_debugfs_add_regset32(struct drm_device *drm, + const char *name, + struct debugfs_regset32 *regset) { - vc4_debugfs_add_file(drm, name, vc4_debugfs_regset32, regset); + return vc4_debugfs_add_file(drm, name, vc4_debugfs_regset32, regset); } diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h index ce12d7ec9c6e..e029dbd4567b 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.h +++ b/drivers/gpu/drm/vc4/vc4_drv.h @@ -865,25 +865,27 @@ void vc4_crtc_get_margins(struct drm_crtc_state *state, /* vc4_debugfs.c */ void vc4_debugfs_init(struct drm_minor *minor); #ifdef CONFIG_DEBUG_FS -void vc4_debugfs_add_file(struct drm_device *drm, - const char *filename, - int (*show)(struct seq_file*, void*), - void *data); -void vc4_debugfs_add_regset32(struct drm_device *drm, - const char *filename, - struct debugfs_regset32 *regset); +int vc4_debugfs_add_file(struct drm_device *drm, + const char *filename, + int (*show)(struct seq_file*, void*), + void *data); +int vc4_debugfs_add_regset32(struct drm_device *drm, + const char *filename, + struct debugfs_regset32 *regset); #else -static inline void vc4_debugfs_add_file(struct drm_device *drm, - const char *filename, - int (*show)(struct seq_file*, void*), - void *data) +static inline int vc4_debugfs_add_file(struct drm_device *drm, + const char *filename, + int (*show)(struct seq_file*, void*), + void *data) { + return 0; }

-static inline void vc4_debugfs_add_regset32(struct drm_device *drm, - const char *filename, - struct debugfs_regset32 *regset) +static inline int vc4_debugfs_add_regset32(struct drm_device *drm, + const char *filename, + struct debugfs_regset32 *regset) { + return 0; } #endif

mutex_init is supposed to be balanced by a call to mutex_destroy that we were never doing in the vc4 driver.

Since a DRM-managed mutex_init variant has been introduced, let's just switch to it.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_bo.c | 15 +++++++++++++-- drivers/gpu/drm/vc4/vc4_drv.c | 4 +++- drivers/gpu/drm/vc4/vc4_gem.c | 10 ++++++++-- drivers/gpu/drm/vc4/vc4_hdmi.c | 5 ++++- 4 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c index 19807e4e0c55..312a1a936caf 100644 --- a/drivers/gpu/drm/vc4/vc4_bo.c +++ b/drivers/gpu/drm/vc4/vc4_bo.c @@ -392,6 +392,7 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size) { struct vc4_dev *vc4 = to_vc4_dev(dev); struct vc4_bo *bo; + int ret;

if (WARN_ON_ONCE(vc4->is_vc5)) return ERR_PTR(-ENODEV); @@ -402,7 +403,11 @@ struct drm_gem_object *vc4_create_object(struct drm_device *dev, size_t size)

bo->madv = VC4_MADV_WILLNEED; refcount_set(&bo->usecnt, 0); - mutex_init(&bo->madv_lock); + + ret = drmm_mutex_init(dev, &bo->madv_lock); + if (ret) + return ERR_PTR(ret); + mutex_lock(&vc4->bo_lock); bo->label = VC4_BO_TYPE_KERNEL; vc4->bo_labels[VC4_BO_TYPE_KERNEL].num_allocated++; @@ -1003,6 +1008,7 @@ static void vc4_bo_cache_destroy(struct drm_device *dev, void *unused); int vc4_bo_cache_init(struct drm_device *dev) { struct vc4_dev *vc4 = to_vc4_dev(dev); + int ret; int i;

if (WARN_ON_ONCE(vc4->is_vc5)) @@ -1022,7 +1028,12 @@ int vc4_bo_cache_init(struct drm_device *dev) for (i = 0; i < VC4_BO_TYPE_COUNT; i++) vc4->bo_labels[i].name = bo_type_names[i];

- mutex_init(&vc4->bo_lock); + ret = drmm_mutex_init(dev, &vc4->bo_lock); + if (ret) { + kfree(vc4->bo_labels); + return ret; + } + INIT_LIST_HEAD(&vc4->bo_cache.time_list);

INIT_WORK(&vc4->bo_cache.time_work, vc4_bo_cache_time_work); diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c index 06d605a12633..c85f86a87bda 100644 --- a/drivers/gpu/drm/vc4/vc4_drv.c +++ b/drivers/gpu/drm/vc4/vc4_drv.c @@ -305,7 +305,9 @@ static int vc4_drm_bind(struct device *dev) INIT_LIST_HEAD(&vc4->debugfs_list);

if (!is_vc5) { - mutex_init(&vc4->bin_bo_lock); + ret = drmm_mutex_init(drm, &vc4->bin_bo_lock); + if (ret) + return ret;

ret = vc4_bo_cache_init(drm); if (ret) diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c index fe10d9c3fff8..7acb43972e69 100644 --- a/drivers/gpu/drm/vc4/vc4_gem.c +++ b/drivers/gpu/drm/vc4/vc4_gem.c @@ -1308,6 +1308,7 @@ static void vc4_gem_destroy(struct drm_device *dev, void *unused); int vc4_gem_init(struct drm_device *dev) { struct vc4_dev *vc4 = to_vc4_dev(dev); + int ret;

if (WARN_ON_ONCE(vc4->is_vc5)) return -ENODEV; @@ -1325,10 +1326,15 @@ int vc4_gem_init(struct drm_device *dev)

INIT_WORK(&vc4->job_done_work, vc4_job_done_work);

- mutex_init(&vc4->power_lock); + ret = drmm_mutex_init(dev, &vc4->power_lock); + if (ret) + return ret;

INIT_LIST_HEAD(&vc4->purgeable.list); - mutex_init(&vc4->purgeable.lock); + + ret = drmm_mutex_init(dev, &vc4->purgeable.lock); + if (ret) + return ret;

return drmm_add_action_or_reset(dev, vc4_gem_destroy, NULL); } diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index ef46dc34316a..8f6531e2a500 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -3196,7 +3196,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) if (!vc4_hdmi) return -ENOMEM;

- mutex_init(&vc4_hdmi->mutex); + ret = drmm_mutex_init(drm, &vc4_hdmi->mutex); + if (ret) + return ret; + spin_lock_init(&vc4_hdmi->hw_lock); INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq);

The vc4_irq_disable(), among other things, will call disable_irq() to complete any in-flight interrupts.

This requires its counterpart, vc4_irq_enable(), to call enable_irq() which causes issues addressed in a later patch.

However, vc4_irq_disable() is called by two callees: vc4_irq_uninstall() and vc4_v3d_runtime_suspend().

vc4_irq_uninstall() also calls free_irq() which already disables the interrupt line. We thus don't require an explicit disable_irq() for that call site.

vc4_v3d_runtime_suspend() doesn't have any other code. However, the rest of vc4_irq_disable() masks the interrupts coming from the v3d, so explictly disabling the interrupt line is also redundant.

The only thing we really care about is thus to make sure we don't have any handler in-flight, as suggested by the comment. We can thus replace disable_irq() by synchronize_irq().

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_irq.c | 2 +- drivers/gpu/drm/vc4/vc4_v3d.c | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c index 2eacfb6773d2..a9fc63d9a7f0 100644 --- a/drivers/gpu/drm/vc4/vc4_irq.c +++ b/drivers/gpu/drm/vc4/vc4_irq.c @@ -295,7 +295,7 @@ vc4_irq_disable(struct drm_device *dev) V3D_WRITE(V3D_INTCTL, V3D_DRIVER_IRQS);

/* Finish any interrupt handler still in flight. */ - disable_irq(vc4->irq); + synchronize_irq(vc4->irq);

cancel_work_sync(&vc4->overflow_mem_work); } diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c index e7990bed0a96..a2da0db73a5c 100644 --- a/drivers/gpu/drm/vc4/vc4_v3d.c +++ b/drivers/gpu/drm/vc4/vc4_v3d.c @@ -393,8 +393,6 @@ static int vc4_v3d_runtime_resume(struct device *dev)

vc4_v3d_init_hw(&vc4->base);

- /* We disabled the IRQ as part of vc4_irq_uninstall in suspend. */ - enable_irq(vc4->irq); vc4_irq_enable(&vc4->base);

return 0;

devm_pm_runtime_enable() simplifies the driver a bit since it will call pm_runtime_disable() automatically through a device-managed action.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/vc4/vc4_v3d.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c index d82c86865079..40f04157ea39 100644 --- a/drivers/gpu/drm/vc4/vc4_v3d.c +++ b/drivers/gpu/drm/vc4/vc4_v3d.c @@ -468,11 +468,13 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data) return ret; vc4->irq = ret;

- pm_runtime_enable(dev); + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret;

ret = pm_runtime_resume_and_get(dev); if (ret) - goto err_disable_runtime_pm; + return ret;

if (V3D_READ(V3D_IDENT0) != V3D_EXPECTED_IDENT0) { DRM_ERROR("V3D_IDENT0 read 0x%08x instead of 0x%08x\n", @@ -501,9 +503,6 @@ static int vc4_v3d_bind(struct device *dev, struct device *master, void *data) err_put_runtime_pm: pm_runtime_put(dev);

-err_disable_runtime_pm: - pm_runtime_disable(dev); - return ret; }

@@ -513,8 +512,6 @@ static void vc4_v3d_unbind(struct device *dev, struct device *master, struct drm_device *drm = dev_get_drvdata(master); struct vc4_dev *vc4 = to_vc4_dev(drm);

- pm_runtime_disable(dev); - vc4_irq_uninstall(drm);

/* Disable the binner's overflow memory address, so the next