dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly
@ 2023-12-06 11:13 Maxime Ripard
  2023-12-06 11:13 ` [PATCH 2/4] drm/crtc: " Maxime Ripard
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Maxime Ripard @ 2023-12-06 11:13 UTC (permalink / raw)
  To: Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: dri-devel

All of the current plane init / allocation functions behave slightly
differently when it comes to argument sanitizing:

 - drm_universal_plane_init() implicitly panics if the drm_device
   pointer or the drm_plane_funcs pointer are NULL, and calls WARN_ON if
   there's no destroy implementation but goes on with the initialization.

 - drm_universal_plane_alloc() implicitly panics if the drm_device
   pointer is NULL, and will call WARN_ON and return an error if the
   drm_plane_funcs pointer is NULL.

 - drmm_universal_plane_alloc() implicitly panics if the drm_device
   pointer is NULL, and will call WARN_ON and return an error if the
   drm_plane_funcs pointer is NULL or if there is a destroy
   implementation.

The current consensus is that the drm_device pointer, the
drm_plane_funcs pointer, and the drm_plane pointer if relevant, should
be considered pre-requisite and the function should panic if we
encounter such a situation, and that returning an error in such a
situation is not welcome.

Let's make all functions consider those three pointers to be always set
and explicitly panic if they aren't. And let's document that behaviour
too.

Link: https://lore.kernel.org/dri-devel/20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org/
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_plane.c | 15 +++++++++++----
 include/drm/drm_plane.h     |  6 ++++++
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 9e8e4c60983d..ce0fa98a0e3f 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -482,6 +482,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @plane or @funcs are NULL.
  */
 int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 			     uint32_t possible_crtcs,
@@ -494,6 +497,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 	va_list ap;
 	int ret;
 
+	BUG_ON(!dev);
+	BUG_ON(!plane);
+	BUG_ON(!funcs);
 	WARN_ON(!funcs->destroy);
 
 	va_start(ap, name);
@@ -528,8 +534,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
 	va_list ap;
 	int ret;
 
-	if (WARN_ON(!funcs || funcs->destroy))
-		return ERR_PTR(-EINVAL);
+	BUG_ON(!dev);
+	BUG_ON(!funcs);
+	WARN_ON(funcs->destroy);
 
 	container = drmm_kzalloc(dev, size, GFP_KERNEL);
 	if (!container)
@@ -567,8 +574,8 @@ void *__drm_universal_plane_alloc(struct drm_device *dev, size_t size,
 	va_list ap;
 	int ret;
 
-	if (drm_WARN_ON(dev, !funcs))
-		return ERR_PTR(-EINVAL);
+	BUG_ON(!dev);
+	BUG_ON(!funcs);
 
 	container = kzalloc(size, GFP_KERNEL);
 	if (!container)
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c6565a6f9324..2dab1b360fa2 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -824,6 +824,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
  *
  * Returns:
  * Pointer to new plane, or ERR_PTR on failure.
+ *
+ * Panics:
+ * If @dev or @funcs are NULL.
  */
 #define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
 				   format_count, format_modifiers, plane_type, name, ...) \
@@ -868,6 +871,9 @@ void *__drm_universal_plane_alloc(struct drm_device *dev,
  *
  * Returns:
  * Pointer to new plane, or ERR_PTR on failure.
+ *
+ * Panics:
+ * If @dev or @funcs are NULL.
  */
 #define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
 				   format_count, format_modifiers, plane_type, name, ...) \
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/4] drm/crtc: Make init functions panic consitently and explicitly
  2023-12-06 11:13 [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly Maxime Ripard
@ 2023-12-06 11:13 ` Maxime Ripard
  2023-12-06 11:13 ` [PATCH 3/4] drm/encoder: " Maxime Ripard
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2023-12-06 11:13 UTC (permalink / raw)
  To: Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: dri-devel

All of the current CRTC init / allocation functions behave slightly
differently when it comes to argument sanitizing:

 - drm_crtc_init_with_planes() implicitly panics if the drm_device
   pointer, the drm_crtc pointer, or the drm_crtc_funcs pointer are
   NULL, and calls WARN_ON if there's no destroy implementation but
   goes on with the initialization.

 - drmm_crtc_init_with_planes() implicitly panics if the drm_device
   pointer, the drm_crtc pointer, or the drm_crtc_funcs pointer are
   NULL, and calls WARN_ON if there's no destroy implementation but
   goes on with the initialization.

 - drmm_crtc_alloc_with_planes() implicitly panics if the drm_device
   pointer, or the drm_crtc pointer are NULL, and calls WARN_ON if
   the drm_crtc_funcs pointer is NULL or there's no destroy
   implementation but goes on with the initialization.

The current consensus is that the drm_device pointer, the
drm_crtc_funcs pointer, and the drm_crtc pointer if relevant, should be
considered pre-requisite and the function should panic if we encounter
such a situation, and that returning an error in such a situation is not
welcome.

Let's make all functions consider those three pointers to be always set
and explicitly panic if they aren't. And let's document that behaviour
too.

Link: https://lore.kernel.org/dri-devel/20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org/
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_crtc.c | 18 ++++++++++++++++--
 include/drm/drm_crtc.h     |  3 +++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index df9bf3c9206e..87e5877b753d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -350,6 +350,9 @@ static int __drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @crtc, or @funcs are NULL.
  */
 int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 			      struct drm_plane *primary,
@@ -360,6 +363,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	va_list ap;
 	int ret;
 
+	BUG_ON(!dev);
+	BUG_ON(!crtc);
+	BUG_ON(!funcs);
 	WARN_ON(!funcs->destroy);
 
 	va_start(ap, name);
@@ -432,6 +438,9 @@ static int __drmm_crtc_init_with_planes(struct drm_device *dev,
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @crtc, or @funcs are NULL.
  */
 int drmm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 			       struct drm_plane *primary,
@@ -442,6 +451,10 @@ int drmm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
 	va_list ap;
 	int ret;
 
+	BUG_ON(!dev);
+	BUG_ON(!crtc);
+	BUG_ON(!funcs);
+
 	va_start(ap, name);
 	ret = __drmm_crtc_init_with_planes(dev, crtc, primary, cursor, funcs,
 					   name, ap);
@@ -465,8 +478,9 @@ void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
 	va_list ap;
 	int ret;
 
-	if (WARN_ON(!funcs || funcs->destroy))
-		return ERR_PTR(-EINVAL);
+	BUG_ON(!dev);
+	BUG_ON(!funcs);
+	WARN_ON(funcs->destroy);
 
 	container = drmm_kzalloc(dev, size, GFP_KERNEL);
 	if (!container)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8b48a1974da3..fdcbc3ac9e08 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1248,6 +1248,9 @@ void *__drmm_crtc_alloc_with_planes(struct drm_device *dev,
  *
  * Returns:
  * Pointer to new crtc, or ERR_PTR on failure.
+ *
+ * Panics:
+ * If @dev or @funcs are NULL.
  */
 #define drmm_crtc_alloc_with_planes(dev, type, member, primary, cursor, funcs, name, ...) \
 	((type *)__drmm_crtc_alloc_with_planes(dev, sizeof(type), \
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/4] drm/encoder: Make init functions panic consitently and explicitly
  2023-12-06 11:13 [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly Maxime Ripard
  2023-12-06 11:13 ` [PATCH 2/4] drm/crtc: " Maxime Ripard
@ 2023-12-06 11:13 ` Maxime Ripard
  2023-12-06 11:13 ` [PATCH 4/4] drm/connector: " Maxime Ripard
  2023-12-07 10:18 ` [PATCH 1/4] drm/plane: " Jani Nikula
  3 siblings, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2023-12-06 11:13 UTC (permalink / raw)
  To: Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: dri-devel

All of the current encoder init / allocation functions behave slightly
differently when it comes to argument sanitizing:

 - drm_encoder_init() implicitly panics if the drm_device pointer, or
   the drm_encoder pointer are NULL, and calls WARN_ON if there's no
   destroy implementation but goes on with the initialization.

 - drmm_encoder_init() implicitly panics if the drm_device pointer is
   NULL, and calls WARN_ON and errors out if the drm_encoder_funcs
   pointer is NULL or if there's no destroy implementation.

 - drmm_encoder_alloct() implicitly panics if the drm_device pointer is
   NULL, and calls WARN_ON and errors out if the drm_encoder_funcs
   pointer is NULL or if there's no destroy implementation.

The current consensus is that the drm_device pointer, the
drm_encoder_funcs pointer, and the drm_encoder pointer if relevant,
should be considered pre-requisite and the function should panic if we
encounter such a situation, and that returning an error in such a
situation is not welcome.

Let's make all functions consider those three pointers to be always set
and explicitly panic if they aren't. And let's document that behaviour
too.

Link: https://lore.kernel.org/dri-devel/20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org/
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_encoder.c | 19 +++++++++++++++++--
 include/drm/drm_encoder.h     |  6 ++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 8f2bc6a28482..bb120b0814a3 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -159,6 +159,9 @@ static int __drm_encoder_init(struct drm_device *dev,
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @encoder, or @funcs are NULL.
  */
 int drm_encoder_init(struct drm_device *dev,
 		     struct drm_encoder *encoder,
@@ -168,6 +171,9 @@ int drm_encoder_init(struct drm_device *dev,
 	va_list ap;
 	int ret;
 
+	BUG_ON(!dev);
+	BUG_ON(!encoder);
+	BUG_ON(!funcs);
 	WARN_ON(!funcs->destroy);
 
 	va_start(ap, name);
@@ -227,8 +233,7 @@ static int __drmm_encoder_init(struct drm_device *dev,
 {
 	int ret;
 
-	if (drm_WARN_ON(dev, funcs && funcs->destroy))
-		return -EINVAL;
+	drm_WARN_ON(dev, funcs->destroy);
 
 	ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, args);
 	if (ret)
@@ -250,6 +255,9 @@ void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset,
 	va_list ap;
 	int ret;
 
+	BUG_ON(!dev);
+	BUG_ON(!funcs);
+
 	container = drmm_kzalloc(dev, size, GFP_KERNEL);
 	if (!container)
 		return ERR_PTR(-ENOMEM);
@@ -283,6 +291,9 @@ EXPORT_SYMBOL(__drmm_encoder_alloc);
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @encoder, or @funcs are NULL.
  */
 int drmm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder,
 		      const struct drm_encoder_funcs *funcs,
@@ -291,6 +302,10 @@ int drmm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder,
 	va_list ap;
 	int ret;
 
+	BUG_ON(!dev);
+	BUG_ON(!encoder);
+	BUG_ON(!funcs);
+
 	va_start(ap, name);
 	ret = __drmm_encoder_init(dev, encoder, funcs, encoder_type, name, ap);
 	va_end(ap);
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 977a9381c8ba..1dbebbc36dd4 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -238,6 +238,9 @@ void *__drmm_encoder_alloc(struct drm_device *dev,
  *
  * Returns:
  * Pointer to new encoder, or ERR_PTR on failure.
+ *
+ * Panics:
+ * If @dev, or @funcs are NULL.
  */
 #define drmm_encoder_alloc(dev, type, member, funcs, encoder_type, name, ...) \
 	((type *)__drmm_encoder_alloc(dev, sizeof(type), \
@@ -256,6 +259,9 @@ void *__drmm_encoder_alloc(struct drm_device *dev,
  *
  * Returns:
  * Pointer to the new drm_encoder struct, or ERR_PTR on failure.
+ *
+ * Panics:
+ * If @dev, or @funcs are NULL.
  */
 #define drmm_plain_encoder_alloc(dev, funcs, encoder_type, name, ...) \
 	((struct drm_encoder *) \
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 4/4] drm/connector: Make init functions panic consitently and explicitly
  2023-12-06 11:13 [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly Maxime Ripard
  2023-12-06 11:13 ` [PATCH 2/4] drm/crtc: " Maxime Ripard
  2023-12-06 11:13 ` [PATCH 3/4] drm/encoder: " Maxime Ripard
@ 2023-12-06 11:13 ` Maxime Ripard
  2023-12-07 10:18 ` [PATCH 1/4] drm/plane: " Jani Nikula
  3 siblings, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2023-12-06 11:13 UTC (permalink / raw)
  To: Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: dri-devel

All of the current encoder init / allocation functions behave slightly
differently when it comes to argument sanitizing:

 - drm_connector_init() and drm_connector_init_with_ddc() implicitly
   panic if the drm_device pointer, or the drm_connector pointer are
   NULL, and will call WARN_ON and error out if the
   drm_connector_funcs pointer is NULL or if there's no destroy
   implementation.

 - drmm_connector_init() implicitly panics if the drm_device pointer,
   the drm_connector_funcs pointer, or the drm_connector pointer are
   NULL, and will call WARN_ON and error out if there's a destroy
   implementation.

The current consensus is that the drm_device pointer, the drm_connector
pointer, and the drm_connector_funcs pointer should be considered
pre-requisite and the function should panic if we encounter such a
situation, and that returning an error in such a situation is not
welcome.

Let's make all functions consider those three pointers to be always set
and explicitly panic if they aren't. And let's document that behaviour
too.

Link: https://lore.kernel.org/dri-devel/20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org/
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_connector.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b0516505f7ae..e7a463db11ea 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -351,14 +351,19 @@ static int __drm_connector_init(struct drm_device *dev,
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @connector, or @funcs are NULL.
  */
 int drm_connector_init(struct drm_device *dev,
 		       struct drm_connector *connector,
 		       const struct drm_connector_funcs *funcs,
 		       int connector_type)
 {
-	if (drm_WARN_ON(dev, !(funcs && funcs->destroy)))
-		return -EINVAL;
+	BUG_ON(!dev);
+	BUG_ON(!connector);
+	BUG_ON(!funcs);
+	drm_WARN_ON(dev, !funcs->destroy);
 
 	return __drm_connector_init(dev, connector, funcs, connector_type, NULL);
 }
@@ -387,6 +392,9 @@ EXPORT_SYMBOL(drm_connector_init);
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @connector, or @funcs are NULL.
  */
 int drm_connector_init_with_ddc(struct drm_device *dev,
 				struct drm_connector *connector,
@@ -394,8 +402,10 @@ int drm_connector_init_with_ddc(struct drm_device *dev,
 				int connector_type,
 				struct i2c_adapter *ddc)
 {
-	if (drm_WARN_ON(dev, !(funcs && funcs->destroy)))
-		return -EINVAL;
+	BUG_ON(!dev);
+	BUG_ON(!connector);
+	BUG_ON(!funcs);
+	drm_WARN_ON(dev, !funcs->destroy);
 
 	return __drm_connector_init(dev, connector, funcs, connector_type, ddc);
 }
@@ -427,6 +437,9 @@ static void drm_connector_cleanup_action(struct drm_device *dev,
  *
  * Returns:
  * Zero on success, error code on failure.
+ *
+ * Panics:
+ * If @dev, @connector, or @funcs are NULL.
  */
 int drmm_connector_init(struct drm_device *dev,
 			struct drm_connector *connector,
@@ -436,8 +449,10 @@ int drmm_connector_init(struct drm_device *dev,
 {
 	int ret;
 
-	if (drm_WARN_ON(dev, funcs && funcs->destroy))
-		return -EINVAL;
+	BUG_ON(!dev);
+	BUG_ON(!connector);
+	BUG_ON(!funcs);
+	drm_WARN_ON(dev, funcs->destroy);
 
 	ret = __drm_connector_init(dev, connector, funcs, connector_type, ddc);
 	if (ret)
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly
  2023-12-06 11:13 [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly Maxime Ripard
                   ` (2 preceding siblings ...)
  2023-12-06 11:13 ` [PATCH 4/4] drm/connector: " Maxime Ripard
@ 2023-12-07 10:18 ` Jani Nikula
  2023-12-07 13:05   ` Maxime Ripard
  3 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2023-12-07 10:18 UTC (permalink / raw)
  To: Maxime Ripard, Daniel Vetter, David Airlie, Maarten Lankhorst,
	Thomas Zimmermann, Maxime Ripard
  Cc: dri-devel

On Wed, 06 Dec 2023, Maxime Ripard <mripard@kernel.org> wrote:
> All of the current plane init / allocation functions behave slightly
> differently when it comes to argument sanitizing:
>
>  - drm_universal_plane_init() implicitly panics if the drm_device
>    pointer or the drm_plane_funcs pointer are NULL, and calls WARN_ON if
>    there's no destroy implementation but goes on with the initialization.
>
>  - drm_universal_plane_alloc() implicitly panics if the drm_device
>    pointer is NULL, and will call WARN_ON and return an error if the
>    drm_plane_funcs pointer is NULL.
>
>  - drmm_universal_plane_alloc() implicitly panics if the drm_device
>    pointer is NULL, and will call WARN_ON and return an error if the
>    drm_plane_funcs pointer is NULL or if there is a destroy
>    implementation.

NULL deref oopses but doesn't necessarily panic, right? Adding BUG() or
BUG_ON() to unconditionally panic is not the way to go, either.

BR,
Jani.


>
> The current consensus is that the drm_device pointer, the
> drm_plane_funcs pointer, and the drm_plane pointer if relevant, should
> be considered pre-requisite and the function should panic if we
> encounter such a situation, and that returning an error in such a
> situation is not welcome.
>
> Let's make all functions consider those three pointers to be always set
> and explicitly panic if they aren't. And let's document that behaviour
> too.
>
> Link: https://lore.kernel.org/dri-devel/20231128-kms-hdmi-connector-state-v4-5-c7602158306e@kernel.org/
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_plane.c | 15 +++++++++++----
>  include/drm/drm_plane.h     |  6 ++++++
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 9e8e4c60983d..ce0fa98a0e3f 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -482,6 +482,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>   *
>   * Returns:
>   * Zero on success, error code on failure.
> + *
> + * Panics:
> + * If @dev, @plane or @funcs are NULL.
>   */
>  int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     uint32_t possible_crtcs,
> @@ -494,6 +497,9 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	va_list ap;
>  	int ret;
>  
> +	BUG_ON(!dev);
> +	BUG_ON(!plane);
> +	BUG_ON(!funcs);
>  	WARN_ON(!funcs->destroy);
>  
>  	va_start(ap, name);
> @@ -528,8 +534,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size,
>  	va_list ap;
>  	int ret;
>  
> -	if (WARN_ON(!funcs || funcs->destroy))
> -		return ERR_PTR(-EINVAL);
> +	BUG_ON(!dev);
> +	BUG_ON(!funcs);
> +	WARN_ON(funcs->destroy);
>  
>  	container = drmm_kzalloc(dev, size, GFP_KERNEL);
>  	if (!container)
> @@ -567,8 +574,8 @@ void *__drm_universal_plane_alloc(struct drm_device *dev, size_t size,
>  	va_list ap;
>  	int ret;
>  
> -	if (drm_WARN_ON(dev, !funcs))
> -		return ERR_PTR(-EINVAL);
> +	BUG_ON(!dev);
> +	BUG_ON(!funcs);
>  
>  	container = kzalloc(size, GFP_KERNEL);
>  	if (!container)
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index c6565a6f9324..2dab1b360fa2 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -824,6 +824,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
>   *
>   * Returns:
>   * Pointer to new plane, or ERR_PTR on failure.
> + *
> + * Panics:
> + * If @dev or @funcs are NULL.
>   */
>  #define drmm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
>  				   format_count, format_modifiers, plane_type, name, ...) \
> @@ -868,6 +871,9 @@ void *__drm_universal_plane_alloc(struct drm_device *dev,
>   *
>   * Returns:
>   * Pointer to new plane, or ERR_PTR on failure.
> + *
> + * Panics:
> + * If @dev or @funcs are NULL.
>   */
>  #define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \
>  				   format_count, format_modifiers, plane_type, name, ...) \

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly
  2023-12-07 10:18 ` [PATCH 1/4] drm/plane: " Jani Nikula
@ 2023-12-07 13:05   ` Maxime Ripard
  0 siblings, 0 replies; 6+ messages in thread
From: Maxime Ripard @ 2023-12-07 13:05 UTC (permalink / raw)
  To: Jani Nikula; +Cc: David Airlie, Daniel Vetter, dri-devel, Thomas Zimmermann

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

On Thu, Dec 07, 2023 at 12:18:13PM +0200, Jani Nikula wrote:
> On Wed, 06 Dec 2023, Maxime Ripard <mripard@kernel.org> wrote:
> > All of the current plane init / allocation functions behave slightly
> > differently when it comes to argument sanitizing:
> >
> >  - drm_universal_plane_init() implicitly panics if the drm_device
> >    pointer or the drm_plane_funcs pointer are NULL, and calls WARN_ON if
> >    there's no destroy implementation but goes on with the initialization.
> >
> >  - drm_universal_plane_alloc() implicitly panics if the drm_device
> >    pointer is NULL, and will call WARN_ON and return an error if the
> >    drm_plane_funcs pointer is NULL.
> >
> >  - drmm_universal_plane_alloc() implicitly panics if the drm_device
> >    pointer is NULL, and will call WARN_ON and return an error if the
> >    drm_plane_funcs pointer is NULL or if there is a destroy
> >    implementation.
> 
> NULL deref oopses but doesn't necessarily panic, right? Adding BUG() or
> BUG_ON() to unconditionally panic is not the way to go, either.

Sigh...

Your whole argument was that it was a precondition, and what the current
code was doing is just like an assertion. An assertion will stop the
execution of the program it's in.

If panicking isn't the way to go, then what is?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-12-07 13:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-06 11:13 [PATCH 1/4] drm/plane: Make init functions panic consitently and explicitly Maxime Ripard
2023-12-06 11:13 ` [PATCH 2/4] drm/crtc: " Maxime Ripard
2023-12-06 11:13 ` [PATCH 3/4] drm/encoder: " Maxime Ripard
2023-12-06 11:13 ` [PATCH 4/4] drm/connector: " Maxime Ripard
2023-12-07 10:18 ` [PATCH 1/4] drm/plane: " Jani Nikula
2023-12-07 13:05   ` Maxime Ripard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).