[PATCH RESEND drm-misc-next 0/7] drm/arm/hdlcd: use drm managed resources

[PATCH RESEND drm-misc-next 0/7] drm/arm/hdlcd: use drm managed resources

[Danilo Krummrich]

Hi,

This patch series converts the driver to use drm managed resources to prevent
potential use-after-free issues on driver unbind/rebind and to get rid of the
usage of deprecated APIs.

Danilo Krummrich (7):
  drm/arm/hdlcd: use drmm_* to allocate driver structures
  drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv()
  drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes()
  drm/arm/hdlcd: plane: use drm managed resources
  drm/arm/hdlcd: use drm_dev_unplug()
  drm/arm/hdlcd: crtc: protect device resources after removal
  drm/arm/hdlcd: debugfs: protect device resources after removal

 drivers/gpu/drm/arm/hdlcd_crtc.c | 78 ++++++++++++++++++++++++--------
 drivers/gpu/drm/arm/hdlcd_drv.c  | 36 ++++++++-------
 drivers/gpu/drm/arm/hdlcd_drv.h  |  2 +
 3 files changed, 79 insertions(+), 37 deletions(-)


base-commit: 8fe444eb326869823f3788a4b4da5dca03339d10
-- 
2.37.2

[PATCH RESEND drm-misc-next 0/7] drm/arm/hdlcd: use drm managed resources

[Danilo Krummrich]

Hi,

This patch series converts the driver to use drm managed resources to prevent
potential use-after-free issues on driver unbind/rebind and to get rid of the
usage of deprecated APIs.

Danilo Krummrich (7):
  drm/arm/hdlcd: use drmm_* to allocate driver structures
  drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv()
  drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes()
  drm/arm/hdlcd: plane: use drm managed resources
  drm/arm/hdlcd: use drm_dev_unplug()
  drm/arm/hdlcd: crtc: protect device resources after removal
  drm/arm/hdlcd: debugfs: protect device resources after removal

 drivers/gpu/drm/arm/hdlcd_crtc.c | 78 ++++++++++++++++++++++++--------
 drivers/gpu/drm/arm/hdlcd_drv.c  | 36 ++++++++-------
 drivers/gpu/drm/arm/hdlcd_drv.h  |  2 +
 3 files changed, 79 insertions(+), 37 deletions(-)


base-commit: 8fe444eb326869823f3788a4b4da5dca03339d10
-- 
2.37.2

[PATCH RESEND drm-misc-next 1/7] drm/arm/hdlcd: use drmm_* to allocate driver structures

[Danilo Krummrich]

Use drm managed resources to allocate driver structures and get rid of
the deprecated drm_dev_alloc() call and replace it with
devm_drm_dev_alloc().

This also serves as preparation to get rid of drm_device->dev_private
and to fix use-after-free issues on driver unload.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c | 12 ++++--------
 drivers/gpu/drm/arm/hdlcd_drv.h |  1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index a032003c340c..463381d11cff 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -247,13 +247,11 @@ static int hdlcd_drm_bind(struct device *dev)
 	struct hdlcd_drm_private *hdlcd;
 	int ret;
 
-	hdlcd = devm_kzalloc(dev, sizeof(*hdlcd), GFP_KERNEL);
-	if (!hdlcd)
-		return -ENOMEM;
+	hdlcd = devm_drm_dev_alloc(dev, &hdlcd_driver, typeof(*hdlcd), base);
+	if (IS_ERR(hdlcd))
+		return PTR_ERR(hdlcd);
 
-	drm = drm_dev_alloc(&hdlcd_driver, dev);
-	if (IS_ERR(drm))
-		return PTR_ERR(drm);
+	drm = &hdlcd->base;
 
 	drm->dev_private = hdlcd;
 	dev_set_drvdata(dev, drm);
@@ -319,7 +317,6 @@ static int hdlcd_drm_bind(struct device *dev)
 err_free:
 	drm_mode_config_cleanup(drm);
 	dev_set_drvdata(dev, NULL);
-	drm_dev_put(drm);
 
 	return ret;
 }
@@ -344,7 +341,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 	drm_mode_config_cleanup(drm);
 	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
-	drm_dev_put(drm);
 }
 
 static const struct component_master_ops hdlcd_master_ops = {
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 909c39c28487..3892b36767ac 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -7,6 +7,7 @@
 #define __HDLCD_DRV_H__
 
 struct hdlcd_drm_private {
+	struct drm_device		base;
 	void __iomem			*mmio;
 	struct clk			*clk;
 	struct drm_crtc			crtc;
-- 
2.37.2

[PATCH RESEND drm-misc-next 1/7] drm/arm/hdlcd: use drmm_* to allocate driver structures

[Danilo Krummrich]

Use drm managed resources to allocate driver structures and get rid of
the deprecated drm_dev_alloc() call and replace it with
devm_drm_dev_alloc().

This also serves as preparation to get rid of drm_device->dev_private
and to fix use-after-free issues on driver unload.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c | 12 ++++--------
 drivers/gpu/drm/arm/hdlcd_drv.h |  1 +
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index a032003c340c..463381d11cff 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -247,13 +247,11 @@ static int hdlcd_drm_bind(struct device *dev)
 	struct hdlcd_drm_private *hdlcd;
 	int ret;
 
-	hdlcd = devm_kzalloc(dev, sizeof(*hdlcd), GFP_KERNEL);
-	if (!hdlcd)
-		return -ENOMEM;
+	hdlcd = devm_drm_dev_alloc(dev, &hdlcd_driver, typeof(*hdlcd), base);
+	if (IS_ERR(hdlcd))
+		return PTR_ERR(hdlcd);
 
-	drm = drm_dev_alloc(&hdlcd_driver, dev);
-	if (IS_ERR(drm))
-		return PTR_ERR(drm);
+	drm = &hdlcd->base;
 
 	drm->dev_private = hdlcd;
 	dev_set_drvdata(dev, drm);
@@ -319,7 +317,6 @@ static int hdlcd_drm_bind(struct device *dev)
 err_free:
 	drm_mode_config_cleanup(drm);
 	dev_set_drvdata(dev, NULL);
-	drm_dev_put(drm);
 
 	return ret;
 }
@@ -344,7 +341,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 	drm_mode_config_cleanup(drm);
 	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
-	drm_dev_put(drm);
 }
 
 static const struct component_master_ops hdlcd_master_ops = {
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 909c39c28487..3892b36767ac 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -7,6 +7,7 @@
 #define __HDLCD_DRV_H__
 
 struct hdlcd_drm_private {
+	struct drm_device		base;
 	void __iomem			*mmio;
 	struct clk			*clk;
 	struct drm_crtc			crtc;
-- 
2.37.2

[PATCH RESEND drm-misc-next 2/7] drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv()

[Danilo Krummrich]

Using drm_device->dev_private is deprecated. Since we've switched to
devm_drm_dev_alloc(), struct drm_device is now embedded in struct
hdlcd_drm_private, hence we can use container_of() to get the struct
drm_device instance instead.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c |  4 ++--
 drivers/gpu/drm/arm/hdlcd_drv.c  | 10 ++++------
 drivers/gpu/drm/arm/hdlcd_drv.h  |  1 +
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 7030339fa232..4a8959d0b2a6 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -275,7 +275,7 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	dest_h = drm_rect_height(&new_plane_state->dst);
 	scanout_start = drm_fb_dma_get_gem_addr(fb, new_plane_state, 0);
 
-	hdlcd = plane->dev->dev_private;
+	hdlcd = drm_to_hdlcd_priv(plane->dev);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, fb->pitches[0]);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
@@ -325,7 +325,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
 
 int hdlcd_setup_crtc(struct drm_device *drm)
 {
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	struct drm_plane *primary;
 	int ret;
 
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 463381d11cff..120c87934a91 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -98,7 +98,7 @@ static void hdlcd_irq_uninstall(struct hdlcd_drm_private *hdlcd)
 
 static int hdlcd_load(struct drm_device *drm, unsigned long flags)
 {
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct resource *res;
 	u32 version;
@@ -190,7 +190,7 @@ static int hdlcd_show_underrun_count(struct seq_file *m, void *arg)
 {
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct drm_device *drm = node->minor->dev;
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 
 	seq_printf(m, "underrun : %d\n", atomic_read(&hdlcd->buffer_underrun_count));
 	seq_printf(m, "dma_end  : %d\n", atomic_read(&hdlcd->dma_end_count));
@@ -203,7 +203,7 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
 {
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct drm_device *drm = node->minor->dev;
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	unsigned long clkrate = clk_get_rate(hdlcd->clk);
 	unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
 
@@ -253,7 +253,6 @@ static int hdlcd_drm_bind(struct device *dev)
 
 	drm = &hdlcd->base;
 
-	drm->dev_private = hdlcd;
 	dev_set_drvdata(dev, drm);
 
 	hdlcd_setup_mode_config(drm);
@@ -324,7 +323,7 @@ static int hdlcd_drm_bind(struct device *dev)
 static void hdlcd_drm_unbind(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 
 	drm_dev_unregister(drm);
 	drm_kms_helper_poll_fini(drm);
@@ -339,7 +338,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 		pm_runtime_disable(dev);
 	of_reserved_mem_device_release(dev);
 	drm_mode_config_cleanup(drm);
-	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
 }
 
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 3892b36767ac..f1c1da2ac2db 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -21,6 +21,7 @@ struct hdlcd_drm_private {
 #endif
 };
 
+#define drm_to_hdlcd_priv(x)	container_of(x, struct hdlcd_drm_private, base)
 #define crtc_to_hdlcd_priv(x)	container_of(x, struct hdlcd_drm_private, crtc)
 
 static inline void hdlcd_write(struct hdlcd_drm_private *hdlcd,
-- 
2.37.2

[PATCH RESEND drm-misc-next 2/7] drm/arm/hdlcd: replace drm->dev_private with drm_to_hdlcd_priv()

[Danilo Krummrich]

Using drm_device->dev_private is deprecated. Since we've switched to
devm_drm_dev_alloc(), struct drm_device is now embedded in struct
hdlcd_drm_private, hence we can use container_of() to get the struct
drm_device instance instead.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c |  4 ++--
 drivers/gpu/drm/arm/hdlcd_drv.c  | 10 ++++------
 drivers/gpu/drm/arm/hdlcd_drv.h  |  1 +
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 7030339fa232..4a8959d0b2a6 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -275,7 +275,7 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	dest_h = drm_rect_height(&new_plane_state->dst);
 	scanout_start = drm_fb_dma_get_gem_addr(fb, new_plane_state, 0);
 
-	hdlcd = plane->dev->dev_private;
+	hdlcd = drm_to_hdlcd_priv(plane->dev);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_LENGTH, fb->pitches[0]);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
@@ -325,7 +325,7 @@ static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
 
 int hdlcd_setup_crtc(struct drm_device *drm)
 {
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	struct drm_plane *primary;
 	int ret;
 
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 463381d11cff..120c87934a91 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -98,7 +98,7 @@ static void hdlcd_irq_uninstall(struct hdlcd_drm_private *hdlcd)
 
 static int hdlcd_load(struct drm_device *drm, unsigned long flags)
 {
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct resource *res;
 	u32 version;
@@ -190,7 +190,7 @@ static int hdlcd_show_underrun_count(struct seq_file *m, void *arg)
 {
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct drm_device *drm = node->minor->dev;
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 
 	seq_printf(m, "underrun : %d\n", atomic_read(&hdlcd->buffer_underrun_count));
 	seq_printf(m, "dma_end  : %d\n", atomic_read(&hdlcd->dma_end_count));
@@ -203,7 +203,7 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
 {
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct drm_device *drm = node->minor->dev;
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	unsigned long clkrate = clk_get_rate(hdlcd->clk);
 	unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
 
@@ -253,7 +253,6 @@ static int hdlcd_drm_bind(struct device *dev)
 
 	drm = &hdlcd->base;
 
-	drm->dev_private = hdlcd;
 	dev_set_drvdata(dev, drm);
 
 	hdlcd_setup_mode_config(drm);
@@ -324,7 +323,7 @@ static int hdlcd_drm_bind(struct device *dev)
 static void hdlcd_drm_unbind(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 
 	drm_dev_unregister(drm);
 	drm_kms_helper_poll_fini(drm);
@@ -339,7 +338,6 @@ static void hdlcd_drm_unbind(struct device *dev)
 		pm_runtime_disable(dev);
 	of_reserved_mem_device_release(dev);
 	drm_mode_config_cleanup(drm);
-	drm->dev_private = NULL;
 	dev_set_drvdata(dev, NULL);
 }
 
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.h b/drivers/gpu/drm/arm/hdlcd_drv.h
index 3892b36767ac..f1c1da2ac2db 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.h
+++ b/drivers/gpu/drm/arm/hdlcd_drv.h
@@ -21,6 +21,7 @@ struct hdlcd_drm_private {
 #endif
 };
 
+#define drm_to_hdlcd_priv(x)	container_of(x, struct hdlcd_drm_private, base)
 #define crtc_to_hdlcd_priv(x)	container_of(x, struct hdlcd_drm_private, crtc)
 
 static inline void hdlcd_write(struct hdlcd_drm_private *hdlcd,
-- 
2.37.2

[PATCH RESEND drm-misc-next 3/7] drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes()

[Danilo Krummrich]

Use drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes()
to get rid of the explicit drm_crtc_cleanup() call.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 4a8959d0b2a6..c0a5ca7f578a 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -42,7 +42,6 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
 
 	/* stop the controller on cleanup */
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
-	drm_crtc_cleanup(crtc);
 }
 
 static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
@@ -333,8 +332,8 @@ int hdlcd_setup_crtc(struct drm_device *drm)
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
-	ret = drm_crtc_init_with_planes(drm, &hdlcd->crtc, primary, NULL,
-					&hdlcd_crtc_funcs, NULL);
+	ret = drmm_crtc_init_with_planes(drm, &hdlcd->crtc, primary, NULL,
+					 &hdlcd_crtc_funcs, NULL);
 	if (ret)
 		return ret;
 
-- 
2.37.2

[PATCH RESEND drm-misc-next 3/7] drm/arm/hdlcd: crtc: use drmm_crtc_init_with_planes()

[Danilo Krummrich]

Use drmm_crtc_init_with_planes() instead of drm_crtc_init_with_planes()
to get rid of the explicit drm_crtc_cleanup() call.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 4a8959d0b2a6..c0a5ca7f578a 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -42,7 +42,6 @@ static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
 
 	/* stop the controller on cleanup */
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
-	drm_crtc_cleanup(crtc);
 }
 
 static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
@@ -333,8 +332,8 @@ int hdlcd_setup_crtc(struct drm_device *drm)
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
-	ret = drm_crtc_init_with_planes(drm, &hdlcd->crtc, primary, NULL,
-					&hdlcd_crtc_funcs, NULL);
+	ret = drmm_crtc_init_with_planes(drm, &hdlcd->crtc, primary, NULL,
+					 &hdlcd_crtc_funcs, NULL);
 	if (ret)
 		return ret;
 
-- 
2.37.2

[PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Use drm managed resource allocation (drmm_universal_plane_alloc()) in
order to get rid of the explicit destroy hook in struct drm_plane_funcs.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index c0a5ca7f578a..17d3ccf12245 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
 static const struct drm_plane_funcs hdlcd_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 	.disable_plane		= drm_atomic_helper_disable_plane,
-	.destroy		= drm_plane_cleanup,
 	.reset			= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
@@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
 
 static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
 {
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	struct drm_plane *plane = NULL;
 	u32 formats[ARRAY_SIZE(supported_formats)], i;
-	int ret;
-
-	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
-	if (!plane)
-		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
 		formats[i] = supported_formats[i].fourcc;
 
-	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
-				       formats, ARRAY_SIZE(formats),
-				       NULL,
-				       DRM_PLANE_TYPE_PRIMARY, NULL);
-	if (ret)
-		return ERR_PTR(ret);
+	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
+					   &hdlcd_plane_funcs,
+					   formats, ARRAY_SIZE(formats),
+					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(plane))
+		return plane;
 
 	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
 	hdlcd->plane = plane;
-- 
2.37.2

[PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Use drm managed resource allocation (drmm_universal_plane_alloc()) in
order to get rid of the explicit destroy hook in struct drm_plane_funcs.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index c0a5ca7f578a..17d3ccf12245 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
 static const struct drm_plane_funcs hdlcd_plane_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 	.disable_plane		= drm_atomic_helper_disable_plane,
-	.destroy		= drm_plane_cleanup,
 	.reset			= drm_atomic_helper_plane_reset,
 	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
@@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
 
 static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
 {
-	struct hdlcd_drm_private *hdlcd = drm->dev_private;
+	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 	struct drm_plane *plane = NULL;
 	u32 formats[ARRAY_SIZE(supported_formats)], i;
-	int ret;
-
-	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
-	if (!plane)
-		return ERR_PTR(-ENOMEM);
 
 	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
 		formats[i] = supported_formats[i].fourcc;
 
-	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
-				       formats, ARRAY_SIZE(formats),
-				       NULL,
-				       DRM_PLANE_TYPE_PRIMARY, NULL);
-	if (ret)
-		return ERR_PTR(ret);
+	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
+					   &hdlcd_plane_funcs,
+					   formats, ARRAY_SIZE(formats),
+					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(plane))
+		return plane;
 
 	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
 	hdlcd->plane = plane;
-- 
2.37.2

[PATCH RESEND drm-misc-next 5/7] drm/arm/hdlcd: use drm_dev_unplug()

[Danilo Krummrich]

When the driver is unbound, there might still be users in userspace
having an open fd and are calling into the driver.

While this is fine for drm managed resources, it is not for resources
bound to the device/driver lifecycle, e.g. clocks or MMIO mappings.

To prevent use-after-free issues we need to protect those resources with
drm_dev_enter() and drm_dev_exit(). This does only work if we indicate
that the drm device was unplugged, hence use drm_dev_unplug() instead of
drm_dev_unregister().

Protecting the particular resources with drm_dev_enter()/drm_dev_exit()
is handled by subsequent patches.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 120c87934a91..e41def6d47cc 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -325,7 +325,7 @@ static void hdlcd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 
-	drm_dev_unregister(drm);
+	drm_dev_unplug(drm);
 	drm_kms_helper_poll_fini(drm);
 	component_unbind_all(dev, drm);
 	of_node_put(hdlcd->crtc.port);
-- 
2.37.2

[PATCH RESEND drm-misc-next 5/7] drm/arm/hdlcd: use drm_dev_unplug()

[Danilo Krummrich]

When the driver is unbound, there might still be users in userspace
having an open fd and are calling into the driver.

While this is fine for drm managed resources, it is not for resources
bound to the device/driver lifecycle, e.g. clocks or MMIO mappings.

To prevent use-after-free issues we need to protect those resources with
drm_dev_enter() and drm_dev_exit(). This does only work if we indicate
that the drm device was unplugged, hence use drm_dev_unplug() instead of
drm_dev_unregister().

Protecting the particular resources with drm_dev_enter()/drm_dev_exit()
is handled by subsequent patches.

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index 120c87934a91..e41def6d47cc 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -325,7 +325,7 @@ static void hdlcd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
 
-	drm_dev_unregister(drm);
+	drm_dev_unplug(drm);
 	drm_kms_helper_poll_fini(drm);
 	component_unbind_all(dev, drm);
 	of_node_put(hdlcd->crtc.port);
-- 
2.37.2

[PATCH RESEND drm-misc-next 6/7] drm/arm/hdlcd: crtc: protect device resources after removal

[Danilo Krummrich]

(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.

However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.

Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 49 ++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 17d3ccf12245..bfc42d4a53c2 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -18,6 +18,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_fb_dma_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_framebuffer.h>
@@ -39,27 +40,47 @@
 static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
 
 	/* stop the controller on cleanup */
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
+
+	drm_dev_exit(idx);
 }
 
 static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
-	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+	unsigned int mask;
+	int idx;
 
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return -ENODEV;
+
+	mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
 	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
 
+	drm_dev_exit(idx);
+
 	return 0;
 }
 
 static void hdlcd_crtc_disable_vblank(struct drm_crtc *crtc)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
-	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+	unsigned int mask;
+	int idx;
 
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
+
+	mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
 	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
+
+	drm_dev_exit(idx);
 }
 
 static const struct drm_crtc_funcs hdlcd_crtc_funcs = {
@@ -170,21 +191,33 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_state *state)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
 
 	clk_prepare_enable(hdlcd->clk);
 	hdlcd_crtc_mode_set_nofb(crtc);
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
 	drm_crtc_vblank_on(crtc);
+
+	drm_dev_exit(idx);
 }
 
 static void hdlcd_crtc_atomic_disable(struct drm_crtc *crtc,
 				      struct drm_atomic_state *state)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
 
 	drm_crtc_vblank_off(crtc);
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
 	clk_disable_unprepare(hdlcd->clk);
+
+	drm_dev_exit(idx);
 }
 
 static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
@@ -192,6 +225,10 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
 	long rate, clk_rate = mode->clock * 1000;
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return MODE_NOCLOCK;
 
 	rate = clk_round_rate(hdlcd->clk, clk_rate);
 	/* 0.1% seems a close enough tolerance for the TDA19988 on Juno */
@@ -200,6 +237,8 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
 		return MODE_NOCLOCK;
 	}
 
+	drm_dev_exit(idx);
+
 	return MODE_OK;
 }
 
@@ -267,6 +306,10 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	struct hdlcd_drm_private *hdlcd;
 	u32 dest_h;
 	dma_addr_t scanout_start;
+	int idx;
+
+	if (!drm_dev_enter(plane->dev, &idx))
+		return;
 
 	if (!fb)
 		return;
@@ -279,6 +322,8 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
+
+	drm_dev_exit(idx);
 }
 
 static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
-- 
2.37.2

[PATCH RESEND drm-misc-next 6/7] drm/arm/hdlcd: crtc: protect device resources after removal

[Danilo Krummrich]

(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.

However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.

Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_crtc.c | 49 ++++++++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
index 17d3ccf12245..bfc42d4a53c2 100644
--- a/drivers/gpu/drm/arm/hdlcd_crtc.c
+++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
@@ -18,6 +18,7 @@
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_fb_dma_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_framebuffer.h>
@@ -39,27 +40,47 @@
 static void hdlcd_crtc_cleanup(struct drm_crtc *crtc)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
 
 	/* stop the controller on cleanup */
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
+
+	drm_dev_exit(idx);
 }
 
 static int hdlcd_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
-	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+	unsigned int mask;
+	int idx;
 
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return -ENODEV;
+
+	mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
 	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask | HDLCD_INTERRUPT_VSYNC);
 
+	drm_dev_exit(idx);
+
 	return 0;
 }
 
 static void hdlcd_crtc_disable_vblank(struct drm_crtc *crtc)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
-	unsigned int mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
+	unsigned int mask;
+	int idx;
 
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
+
+	mask = hdlcd_read(hdlcd, HDLCD_REG_INT_MASK);
 	hdlcd_write(hdlcd, HDLCD_REG_INT_MASK, mask & ~HDLCD_INTERRUPT_VSYNC);
+
+	drm_dev_exit(idx);
 }
 
 static const struct drm_crtc_funcs hdlcd_crtc_funcs = {
@@ -170,21 +191,33 @@ static void hdlcd_crtc_atomic_enable(struct drm_crtc *crtc,
 				     struct drm_atomic_state *state)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
 
 	clk_prepare_enable(hdlcd->clk);
 	hdlcd_crtc_mode_set_nofb(crtc);
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 1);
 	drm_crtc_vblank_on(crtc);
+
+	drm_dev_exit(idx);
 }
 
 static void hdlcd_crtc_atomic_disable(struct drm_crtc *crtc,
 				      struct drm_atomic_state *state)
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return;
 
 	drm_crtc_vblank_off(crtc);
 	hdlcd_write(hdlcd, HDLCD_REG_COMMAND, 0);
 	clk_disable_unprepare(hdlcd->clk);
+
+	drm_dev_exit(idx);
 }
 
 static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
@@ -192,6 +225,10 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
 {
 	struct hdlcd_drm_private *hdlcd = crtc_to_hdlcd_priv(crtc);
 	long rate, clk_rate = mode->clock * 1000;
+	int idx;
+
+	if (!drm_dev_enter(crtc->dev, &idx))
+		return MODE_NOCLOCK;
 
 	rate = clk_round_rate(hdlcd->clk, clk_rate);
 	/* 0.1% seems a close enough tolerance for the TDA19988 on Juno */
@@ -200,6 +237,8 @@ static enum drm_mode_status hdlcd_crtc_mode_valid(struct drm_crtc *crtc,
 		return MODE_NOCLOCK;
 	}
 
+	drm_dev_exit(idx);
+
 	return MODE_OK;
 }
 
@@ -267,6 +306,10 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	struct hdlcd_drm_private *hdlcd;
 	u32 dest_h;
 	dma_addr_t scanout_start;
+	int idx;
+
+	if (!drm_dev_enter(plane->dev, &idx))
+		return;
 
 	if (!fb)
 		return;
@@ -279,6 +322,8 @@ static void hdlcd_plane_atomic_update(struct drm_plane *plane,
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_PITCH, fb->pitches[0]);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_LINE_COUNT, dest_h - 1);
 	hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
+
+	drm_dev_exit(idx);
 }
 
 static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
-- 
2.37.2

[PATCH RESEND drm-misc-next 7/7] drm/arm/hdlcd: debugfs: protect device resources after removal

[Danilo Krummrich]

(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.

However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.

Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e41def6d47cc..020c7d0c70a5 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -204,11 +204,19 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct drm_device *drm = node->minor->dev;
 	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
-	unsigned long clkrate = clk_get_rate(hdlcd->clk);
-	unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
+	unsigned long clkrate, mode_clock;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return -ENODEV;
+
+	clkrate = clk_get_rate(hdlcd->clk);
+	mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
 
 	seq_printf(m, "hw  : %lu\n", clkrate);
 	seq_printf(m, "mode: %lu\n", mode_clock);
+
+	drm_dev_exit(idx);
 	return 0;
 }
 
-- 
2.37.2

[PATCH RESEND drm-misc-next 7/7] drm/arm/hdlcd: debugfs: protect device resources after removal

[Danilo Krummrich]

(Hardware) resources which are bound to the driver and device lifecycle
must not be accessed after the device and driver are unbound.

However, the DRM device isn't freed as long as the last user didn't
close it, hence userspace can still call into the driver.

Therefore protect the critical sections which are accessing those
resources with drm_dev_enter() and drm_dev_exit().

Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
 drivers/gpu/drm/arm/hdlcd_drv.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index e41def6d47cc..020c7d0c70a5 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -204,11 +204,19 @@ static int hdlcd_show_pxlclock(struct seq_file *m, void *arg)
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct drm_device *drm = node->minor->dev;
 	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
-	unsigned long clkrate = clk_get_rate(hdlcd->clk);
-	unsigned long mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
+	unsigned long clkrate, mode_clock;
+	int idx;
+
+	if (!drm_dev_enter(drm, &idx))
+		return -ENODEV;
+
+	clkrate = clk_get_rate(hdlcd->clk);
+	mode_clock = hdlcd->crtc.mode.crtc_clock * 1000;
 
 	seq_printf(m, "hw  : %lu\n", clkrate);
 	seq_printf(m, "mode: %lu\n", mode_clock);
+
+	drm_dev_exit(idx);
 	return 0;
 }
 
-- 
2.37.2

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

Hi Danilo,

I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
and on start up I get a warning:

[   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
[   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]

It looks like the .destroy hook is still required or I'm missing some other required
series where the WARN has been removed?

Best regards,
Liviu

On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
> Use drm managed resource allocation (drmm_universal_plane_alloc()) in
> order to get rid of the explicit destroy hook in struct drm_plane_funcs.
> 
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index c0a5ca7f578a..17d3ccf12245 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
>  static const struct drm_plane_funcs hdlcd_plane_funcs = {
>  	.update_plane		= drm_atomic_helper_update_plane,
>  	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.destroy		= drm_plane_cleanup,
>  	.reset			= drm_atomic_helper_plane_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
>  
>  static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>  {
> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
>  	struct drm_plane *plane = NULL;
>  	u32 formats[ARRAY_SIZE(supported_formats)], i;
> -	int ret;
> -
> -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> -	if (!plane)
> -		return ERR_PTR(-ENOMEM);
>  
>  	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
>  		formats[i] = supported_formats[i].fourcc;
>  
> -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
> -				       formats, ARRAY_SIZE(formats),
> -				       NULL,
> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
> +					   &hdlcd_plane_funcs,
> +					   formats, ARRAY_SIZE(formats),
> +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (IS_ERR(plane))
> +		return plane;
>  
>  	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
>  	hdlcd->plane = plane;
> -- 
> 2.37.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

Hi Danilo,

I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
and on start up I get a warning:

[   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
[   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]

It looks like the .destroy hook is still required or I'm missing some other required
series where the WARN has been removed?

Best regards,
Liviu

On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
> Use drm managed resource allocation (drmm_universal_plane_alloc()) in
> order to get rid of the explicit destroy hook in struct drm_plane_funcs.
> 
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> index c0a5ca7f578a..17d3ccf12245 100644
> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
>  static const struct drm_plane_funcs hdlcd_plane_funcs = {
>  	.update_plane		= drm_atomic_helper_update_plane,
>  	.disable_plane		= drm_atomic_helper_disable_plane,
> -	.destroy		= drm_plane_cleanup,
>  	.reset			= drm_atomic_helper_plane_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>  	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
>  
>  static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>  {
> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
>  	struct drm_plane *plane = NULL;
>  	u32 formats[ARRAY_SIZE(supported_formats)], i;
> -	int ret;
> -
> -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> -	if (!plane)
> -		return ERR_PTR(-ENOMEM);
>  
>  	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
>  		formats[i] = supported_formats[i].fourcc;
>  
> -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
> -				       formats, ARRAY_SIZE(formats),
> -				       NULL,
> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
> +					   &hdlcd_plane_funcs,
> +					   formats, ARRAY_SIZE(formats),
> +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (IS_ERR(plane))
> +		return plane;
>  
>  	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
>  	hdlcd->plane = plane;
> -- 
> 2.37.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Hi Liviu,

Thanks for having a look!

This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: 
use drmm_crtc_init_with_planes()".

And there it's the other way around. When using 
drmm_crtc_init_with_planes() we shouldn't have a destroy hook in place, 
that's the whole purpose of drmm_crtc_init_with_planes().

We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use 
drmm_crtc_init_with_planes()", it's wrong.

Do you want me to send a v2 for that?

- Danilo



On 9/12/22 19:36, Liviu Dudau wrote:
> Hi Danilo,
> 
> I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> and on start up I get a warning:
> 
> [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> 
> It looks like the .destroy hook is still required or I'm missing some other required
> series where the WARN has been removed?
> 
> Best regards,
> Liviu
> 
> On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
>> Use drm managed resource allocation (drmm_universal_plane_alloc()) in
>> order to get rid of the explicit destroy hook in struct drm_plane_funcs.
>>
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>> ---
>>   drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
>>   1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> index c0a5ca7f578a..17d3ccf12245 100644
>> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
>> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
>>   static const struct drm_plane_funcs hdlcd_plane_funcs = {
>>   	.update_plane		= drm_atomic_helper_update_plane,
>>   	.disable_plane		= drm_atomic_helper_disable_plane,
>> -	.destroy		= drm_plane_cleanup,
>>   	.reset			= drm_atomic_helper_plane_reset,
>>   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>   	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
>> @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
>>   
>>   static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>>   {
>> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
>>   	struct drm_plane *plane = NULL;
>>   	u32 formats[ARRAY_SIZE(supported_formats)], i;
>> -	int ret;
>> -
>> -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>> -	if (!plane)
>> -		return ERR_PTR(-ENOMEM);
>>   
>>   	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
>>   		formats[i] = supported_formats[i].fourcc;
>>   
>> -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
>> -				       formats, ARRAY_SIZE(formats),
>> -				       NULL,
>> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
>> -	if (ret)
>> -		return ERR_PTR(ret);
>> +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
>> +					   &hdlcd_plane_funcs,
>> +					   formats, ARRAY_SIZE(formats),
>> +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> +	if (IS_ERR(plane))
>> +		return plane;
>>   
>>   	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
>>   	hdlcd->plane = plane;
>> -- 
>> 2.37.2
>>
> 

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Hi Liviu,

Thanks for having a look!

This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: 
use drmm_crtc_init_with_planes()".

And there it's the other way around. When using 
drmm_crtc_init_with_planes() we shouldn't have a destroy hook in place, 
that's the whole purpose of drmm_crtc_init_with_planes().

We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use 
drmm_crtc_init_with_planes()", it's wrong.

Do you want me to send a v2 for that?

- Danilo



On 9/12/22 19:36, Liviu Dudau wrote:
> Hi Danilo,
> 
> I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> and on start up I get a warning:
> 
> [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> 
> It looks like the .destroy hook is still required or I'm missing some other required
> series where the WARN has been removed?
> 
> Best regards,
> Liviu
> 
> On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
>> Use drm managed resource allocation (drmm_universal_plane_alloc()) in
>> order to get rid of the explicit destroy hook in struct drm_plane_funcs.
>>
>> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
>> ---
>>   drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
>>   1 file changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> index c0a5ca7f578a..17d3ccf12245 100644
>> --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
>> +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
>> @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
>>   static const struct drm_plane_funcs hdlcd_plane_funcs = {
>>   	.update_plane		= drm_atomic_helper_update_plane,
>>   	.disable_plane		= drm_atomic_helper_disable_plane,
>> -	.destroy		= drm_plane_cleanup,
>>   	.reset			= drm_atomic_helper_plane_reset,
>>   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
>>   	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
>> @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
>>   
>>   static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
>>   {
>> -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
>> +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
>>   	struct drm_plane *plane = NULL;
>>   	u32 formats[ARRAY_SIZE(supported_formats)], i;
>> -	int ret;
>> -
>> -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
>> -	if (!plane)
>> -		return ERR_PTR(-ENOMEM);
>>   
>>   	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
>>   		formats[i] = supported_formats[i].fourcc;
>>   
>> -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
>> -				       formats, ARRAY_SIZE(formats),
>> -				       NULL,
>> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
>> -	if (ret)
>> -		return ERR_PTR(ret);
>> +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
>> +					   &hdlcd_plane_funcs,
>> +					   formats, ARRAY_SIZE(formats),
>> +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>> +	if (IS_ERR(plane))
>> +		return plane;
>>   
>>   	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
>>   	hdlcd->plane = plane;
>> -- 
>> 2.37.2
>>
> 

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
> Hi Liviu,

Hi Danilo,

> 
> Thanks for having a look!
> 
> This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
> drmm_crtc_init_with_planes()".

Agree! However, this is the patch that removes the .destroy hook, so I've replied here.

> 
> And there it's the other way around. When using drmm_crtc_init_with_planes()
> we shouldn't have a destroy hook in place, that's the whole purpose of
> drmm_crtc_init_with_planes().
> 
> We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
> drmm_crtc_init_with_planes()", it's wrong.

So we end up with mixed use of managed and unmanaged APIs?

> 
> Do you want me to send a v2 for that?

Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.

BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources, 
however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
to me again what are you trying to fix?

Best regards,
Liviu


> 
> - Danilo
> 
> 
> 
> On 9/12/22 19:36, Liviu Dudau wrote:
> > Hi Danilo,
> > 
> > I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> > and on start up I get a warning:
> > 
> > [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> > [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> > 
> > It looks like the .destroy hook is still required or I'm missing some other required
> > series where the WARN has been removed?
> > 
> > Best regards,
> > Liviu
> > 
> > On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
> > > Use drm managed resource allocation (drmm_universal_plane_alloc()) in
> > > order to get rid of the explicit destroy hook in struct drm_plane_funcs.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >   drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
> > >   1 file changed, 7 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > index c0a5ca7f578a..17d3ccf12245 100644
> > > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
> > >   static const struct drm_plane_funcs hdlcd_plane_funcs = {
> > >   	.update_plane		= drm_atomic_helper_update_plane,
> > >   	.disable_plane		= drm_atomic_helper_disable_plane,
> > > -	.destroy		= drm_plane_cleanup,
> > >   	.reset			= drm_atomic_helper_plane_reset,
> > >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >   	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> > > @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
> > >   static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
> > >   {
> > > -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> > > +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
> > >   	struct drm_plane *plane = NULL;
> > >   	u32 formats[ARRAY_SIZE(supported_formats)], i;
> > > -	int ret;
> > > -
> > > -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> > > -	if (!plane)
> > > -		return ERR_PTR(-ENOMEM);
> > >   	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
> > >   		formats[i] = supported_formats[i].fourcc;
> > > -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
> > > -				       formats, ARRAY_SIZE(formats),
> > > -				       NULL,
> > > -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> > > -	if (ret)
> > > -		return ERR_PTR(ret);
> > > +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
> > > +					   &hdlcd_plane_funcs,
> > > +					   formats, ARRAY_SIZE(formats),
> > > +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> > > +	if (IS_ERR(plane))
> > > +		return plane;
> > >   	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
> > >   	hdlcd->plane = plane;
> > > -- 
> > > 2.37.2
> > > 
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
> Hi Liviu,

Hi Danilo,

> 
> Thanks for having a look!
> 
> This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
> drmm_crtc_init_with_planes()".

Agree! However, this is the patch that removes the .destroy hook, so I've replied here.

> 
> And there it's the other way around. When using drmm_crtc_init_with_planes()
> we shouldn't have a destroy hook in place, that's the whole purpose of
> drmm_crtc_init_with_planes().
> 
> We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
> drmm_crtc_init_with_planes()", it's wrong.

So we end up with mixed use of managed and unmanaged APIs?

> 
> Do you want me to send a v2 for that?

Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.

BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources, 
however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
to me again what are you trying to fix?

Best regards,
Liviu


> 
> - Danilo
> 
> 
> 
> On 9/12/22 19:36, Liviu Dudau wrote:
> > Hi Danilo,
> > 
> > I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> > and on start up I get a warning:
> > 
> > [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> > [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> > 
> > It looks like the .destroy hook is still required or I'm missing some other required
> > series where the WARN has been removed?
> > 
> > Best regards,
> > Liviu
> > 
> > On Mon, Sep 05, 2022 at 05:27:16PM +0200, Danilo Krummrich wrote:
> > > Use drm managed resource allocation (drmm_universal_plane_alloc()) in
> > > order to get rid of the explicit destroy hook in struct drm_plane_funcs.
> > > 
> > > Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> > > ---
> > >   drivers/gpu/drm/arm/hdlcd_crtc.c | 20 +++++++-------------
> > >   1 file changed, 7 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > index c0a5ca7f578a..17d3ccf12245 100644
> > > --- a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > +++ b/drivers/gpu/drm/arm/hdlcd_crtc.c
> > > @@ -289,7 +289,6 @@ static const struct drm_plane_helper_funcs hdlcd_plane_helper_funcs = {
> > >   static const struct drm_plane_funcs hdlcd_plane_funcs = {
> > >   	.update_plane		= drm_atomic_helper_update_plane,
> > >   	.disable_plane		= drm_atomic_helper_disable_plane,
> > > -	.destroy		= drm_plane_cleanup,
> > >   	.reset			= drm_atomic_helper_plane_reset,
> > >   	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> > >   	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> > > @@ -297,24 +296,19 @@ static const struct drm_plane_funcs hdlcd_plane_funcs = {
> > >   static struct drm_plane *hdlcd_plane_init(struct drm_device *drm)
> > >   {
> > > -	struct hdlcd_drm_private *hdlcd = drm->dev_private;
> > > +	struct hdlcd_drm_private *hdlcd = drm_to_hdlcd_priv(drm);
> > >   	struct drm_plane *plane = NULL;
> > >   	u32 formats[ARRAY_SIZE(supported_formats)], i;
> > > -	int ret;
> > > -
> > > -	plane = devm_kzalloc(drm->dev, sizeof(*plane), GFP_KERNEL);
> > > -	if (!plane)
> > > -		return ERR_PTR(-ENOMEM);
> > >   	for (i = 0; i < ARRAY_SIZE(supported_formats); i++)
> > >   		formats[i] = supported_formats[i].fourcc;
> > > -	ret = drm_universal_plane_init(drm, plane, 0xff, &hdlcd_plane_funcs,
> > > -				       formats, ARRAY_SIZE(formats),
> > > -				       NULL,
> > > -				       DRM_PLANE_TYPE_PRIMARY, NULL);
> > > -	if (ret)
> > > -		return ERR_PTR(ret);
> > > +	plane = drmm_universal_plane_alloc(drm, struct drm_plane, dev, 0xff,
> > > +					   &hdlcd_plane_funcs,
> > > +					   formats, ARRAY_SIZE(formats),
> > > +					   NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> > > +	if (IS_ERR(plane))
> > > +		return plane;
> > >   	drm_plane_helper_add(plane, &hdlcd_plane_helper_funcs);
> > >   	hdlcd->plane = plane;
> > > -- 
> > > 2.37.2
> > > 
> > 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

On 9/13/22 10:58, Liviu Dudau wrote:
> On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
>> Hi Liviu,
> 
> Hi Danilo,
> 
>>
>> Thanks for having a look!
>>
>> This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
>> drmm_crtc_init_with_planes()".
> 
> Agree! However, this is the patch that removes the .destroy hook, so I've replied here.

This is a different .destroy hook, it's the struct drm_plane_funcs one, 
not the struct drm_crtc_funcs one, which the warning is about. Anyway, 
as said, we can just drop the mentioned patch. :-)

> 
>>
>> And there it's the other way around. When using drmm_crtc_init_with_planes()
>> we shouldn't have a destroy hook in place, that's the whole purpose of
>> drmm_crtc_init_with_planes().
>>
>> We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
>> drmm_crtc_init_with_planes()", it's wrong.
> 
> So we end up with mixed use of managed and unmanaged APIs?

In this case, yes. However, I don't think this makes it inconsistent. 
They only thing drmm_crtc_init_with_planes() does different than 
drm_crtc_init_with_planes() is that it set's things up to automatically 
call drm_crtc_cleanup() on .destroy. Since this driver also does a 
register write in the .destroy callback and hence we can't get rid of 
the callback we can just keep it as it is.

> 
>>
>> Do you want me to send a v2 for that?
> 
> Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.
> 
> BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources,
> however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
> hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
> to me again what are you trying to fix?

Sure! DRM managed resources are cleaned up whenever the last reference 
is put. This is not necessarily the case when the driver is unbound, 
hence there might still be calls into the driver and therefore we must 
protect resources that are bound to the driver/device lifecycle (e.g. a 
MMIO region mapped via devm_ioremap_resource()) from being accessed. 
That's why the hdlcd_write() and hdlcd_read() calls in the crtc 
callbacks need to be protected.

However, of course, the changes needed to achieve that should not result 
into hanging rmmod. Unfortunately, just by looking at the patches again 
I don't see how this could happen.

Do you mind trying again with my v2 (although v2 shouldn't make a 
difference for this issue) and provide the back-trace when it hangs?

Thanks,
Danilo

> 
> Best regards,
> Liviu
> 
> 
>>
>> - Danilo
>>
>>
>>
>> On 9/12/22 19:36, Liviu Dudau wrote:
>>> Hi Danilo,
>>>
>>> I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
>>> and on start up I get a warning:
>>>
>>> [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
>>> [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
>>>
>>> It looks like the .destroy hook is still required or I'm missing some other required
>>> series where the WARN has been removed?
>>>
>>> Best regards,
>>> Liviu

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

On 9/13/22 10:58, Liviu Dudau wrote:
> On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
>> Hi Liviu,
> 
> Hi Danilo,
> 
>>
>> Thanks for having a look!
>>
>> This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
>> drmm_crtc_init_with_planes()".
> 
> Agree! However, this is the patch that removes the .destroy hook, so I've replied here.

This is a different .destroy hook, it's the struct drm_plane_funcs one, 
not the struct drm_crtc_funcs one, which the warning is about. Anyway, 
as said, we can just drop the mentioned patch. :-)

> 
>>
>> And there it's the other way around. When using drmm_crtc_init_with_planes()
>> we shouldn't have a destroy hook in place, that's the whole purpose of
>> drmm_crtc_init_with_planes().
>>
>> We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
>> drmm_crtc_init_with_planes()", it's wrong.
> 
> So we end up with mixed use of managed and unmanaged APIs?

In this case, yes. However, I don't think this makes it inconsistent. 
They only thing drmm_crtc_init_with_planes() does different than 
drm_crtc_init_with_planes() is that it set's things up to automatically 
call drm_crtc_cleanup() on .destroy. Since this driver also does a 
register write in the .destroy callback and hence we can't get rid of 
the callback we can just keep it as it is.

> 
>>
>> Do you want me to send a v2 for that?
> 
> Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.
> 
> BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources,
> however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
> hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
> to me again what are you trying to fix?

Sure! DRM managed resources are cleaned up whenever the last reference 
is put. This is not necessarily the case when the driver is unbound, 
hence there might still be calls into the driver and therefore we must 
protect resources that are bound to the driver/device lifecycle (e.g. a 
MMIO region mapped via devm_ioremap_resource()) from being accessed. 
That's why the hdlcd_write() and hdlcd_read() calls in the crtc 
callbacks need to be protected.

However, of course, the changes needed to achieve that should not result 
into hanging rmmod. Unfortunately, just by looking at the patches again 
I don't see how this could happen.

Do you mind trying again with my v2 (although v2 shouldn't make a 
difference for this issue) and provide the back-trace when it hangs?

Thanks,
Danilo

> 
> Best regards,
> Liviu
> 
> 
>>
>> - Danilo
>>
>>
>>
>> On 9/12/22 19:36, Liviu Dudau wrote:
>>> Hi Danilo,
>>>
>>> I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
>>> and on start up I get a warning:
>>>
>>> [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
>>> [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
>>>
>>> It looks like the .destroy hook is still required or I'm missing some other required
>>> series where the WARN has been removed?
>>>
>>> Best regards,
>>> Liviu

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

On Wed, Sep 14, 2022 at 12:03:58AM +0200, Danilo Krummrich wrote:
> On 9/13/22 10:58, Liviu Dudau wrote:
> > On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
> > > Hi Liviu,
> > 
> > Hi Danilo,
> > 
> > > 
> > > Thanks for having a look!
> > > 
> > > This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()".
> > 
> > Agree! However, this is the patch that removes the .destroy hook, so I've replied here.
> 
> This is a different .destroy hook, it's the struct drm_plane_funcs one, not
> the struct drm_crtc_funcs one, which the warning is about. Anyway, as said,
> we can just drop the mentioned patch. :-)

Sorry, my mistake.

> 
> > 
> > > 
> > > And there it's the other way around. When using drmm_crtc_init_with_planes()
> > > we shouldn't have a destroy hook in place, that's the whole purpose of
> > > drmm_crtc_init_with_planes().
> > > 
> > > We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()", it's wrong.
> > 
> > So we end up with mixed use of managed and unmanaged APIs?
> 
> In this case, yes. However, I don't think this makes it inconsistent. They
> only thing drmm_crtc_init_with_planes() does different than
> drm_crtc_init_with_planes() is that it set's things up to automatically call
> drm_crtc_cleanup() on .destroy. Since this driver also does a register write
> in the .destroy callback and hence we can't get rid of the callback we can
> just keep it as it is.

I'm trying to test your v2 on a flaky platform (my Juno R1 has developed some memory
issues that leads to crashes on most boots) and I'm seeing some issues that I'm
trying to replicate (and understand) before posting the stack trace. I'm not sure yet
if they are related to the mixing of managed and unmanaged APIs, I need a bit more
time for testing.


> 
> > 
> > > 
> > > Do you want me to send a v2 for that?
> > 
> > Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.
> > 
> > BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources,
> > however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
> > hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
> > to me again what are you trying to fix?
> 
> Sure! DRM managed resources are cleaned up whenever the last reference is
> put. This is not necessarily the case when the driver is unbound, hence
> there might still be calls into the driver and therefore we must protect
> resources that are bound to the driver/device lifecycle (e.g. a MMIO region
> mapped via devm_ioremap_resource()) from being accessed. That's why the
> hdlcd_write() and hdlcd_read() calls in the crtc callbacks need to be
> protected.

Thanks for the explanation on what the code ultimately does. I was trying to
understand what was the impetus for the change in the first place. Why did you
thought adding the calls to managed APIs was needed and what were you trying to fix?


> 
> However, of course, the changes needed to achieve that should not result
> into hanging rmmod. Unfortunately, just by looking at the patches again I
> don't see how this could happen.
> 
> Do you mind trying again with my v2 (although v2 shouldn't make a difference
> for this issue) and provide the back-trace when it hangs?

I'm trying to do that. I've got one good trace that I'm trying to reproduce, but
unfortunately my main Juno board has developed a memory fault and the replacement for
it is taking longer than I wanted.

Best regards,
Liviu


> 
> Thanks,
> Danilo
> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > - Danilo
> > > 
> > > 
> > > 
> > > On 9/12/22 19:36, Liviu Dudau wrote:
> > > > Hi Danilo,
> > > > 
> > > > I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> > > > and on start up I get a warning:
> > > > 
> > > > [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> > > > [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> > > > 
> > > > It looks like the .destroy hook is still required or I'm missing some other required
> > > > series where the WARN has been removed?
> > > > 
> > > > Best regards,
> > > > Liviu
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

On Wed, Sep 14, 2022 at 12:03:58AM +0200, Danilo Krummrich wrote:
> On 9/13/22 10:58, Liviu Dudau wrote:
> > On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
> > > Hi Liviu,
> > 
> > Hi Danilo,
> > 
> > > 
> > > Thanks for having a look!
> > > 
> > > This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()".
> > 
> > Agree! However, this is the patch that removes the .destroy hook, so I've replied here.
> 
> This is a different .destroy hook, it's the struct drm_plane_funcs one, not
> the struct drm_crtc_funcs one, which the warning is about. Anyway, as said,
> we can just drop the mentioned patch. :-)

Sorry, my mistake.

> 
> > 
> > > 
> > > And there it's the other way around. When using drmm_crtc_init_with_planes()
> > > we shouldn't have a destroy hook in place, that's the whole purpose of
> > > drmm_crtc_init_with_planes().
> > > 
> > > We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()", it's wrong.
> > 
> > So we end up with mixed use of managed and unmanaged APIs?
> 
> In this case, yes. However, I don't think this makes it inconsistent. They
> only thing drmm_crtc_init_with_planes() does different than
> drm_crtc_init_with_planes() is that it set's things up to automatically call
> drm_crtc_cleanup() on .destroy. Since this driver also does a register write
> in the .destroy callback and hence we can't get rid of the callback we can
> just keep it as it is.

I'm trying to test your v2 on a flaky platform (my Juno R1 has developed some memory
issues that leads to crashes on most boots) and I'm seeing some issues that I'm
trying to replicate (and understand) before posting the stack trace. I'm not sure yet
if they are related to the mixing of managed and unmanaged APIs, I need a bit more
time for testing.


> 
> > 
> > > 
> > > Do you want me to send a v2 for that?
> > 
> > Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.
> > 
> > BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources,
> > however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
> > hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
> > to me again what are you trying to fix?
> 
> Sure! DRM managed resources are cleaned up whenever the last reference is
> put. This is not necessarily the case when the driver is unbound, hence
> there might still be calls into the driver and therefore we must protect
> resources that are bound to the driver/device lifecycle (e.g. a MMIO region
> mapped via devm_ioremap_resource()) from being accessed. That's why the
> hdlcd_write() and hdlcd_read() calls in the crtc callbacks need to be
> protected.

Thanks for the explanation on what the code ultimately does. I was trying to
understand what was the impetus for the change in the first place. Why did you
thought adding the calls to managed APIs was needed and what were you trying to fix?


> 
> However, of course, the changes needed to achieve that should not result
> into hanging rmmod. Unfortunately, just by looking at the patches again I
> don't see how this could happen.
> 
> Do you mind trying again with my v2 (although v2 shouldn't make a difference
> for this issue) and provide the back-trace when it hangs?

I'm trying to do that. I've got one good trace that I'm trying to reproduce, but
unfortunately my main Juno board has developed a memory fault and the replacement for
it is taking longer than I wanted.

Best regards,
Liviu


> 
> Thanks,
> Danilo
> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > - Danilo
> > > 
> > > 
> > > 
> > > On 9/12/22 19:36, Liviu Dudau wrote:
> > > > Hi Danilo,
> > > > 
> > > > I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> > > > and on start up I get a warning:
> > > > 
> > > > [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> > > > [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> > > > 
> > > > It looks like the .destroy hook is still required or I'm missing some other required
> > > > series where the WARN has been removed?
> > > > 
> > > > Best regards,
> > > > Liviu
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

On Wed, Sep 14, 2022 at 12:03:58AM +0200, Danilo Krummrich wrote:
> On 9/13/22 10:58, Liviu Dudau wrote:
> > On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
> > > Hi Liviu,
> > 
> > Hi Danilo,
> > 
> > > 
> > > Thanks for having a look!
> > > 
> > > This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()".
> > 
> > Agree! However, this is the patch that removes the .destroy hook, so I've replied here.
> 
> This is a different .destroy hook, it's the struct drm_plane_funcs one, not
> the struct drm_crtc_funcs one, which the warning is about. Anyway, as said,
> we can just drop the mentioned patch. :-)
> 
> > 
> > > 
> > > And there it's the other way around. When using drmm_crtc_init_with_planes()
> > > we shouldn't have a destroy hook in place, that's the whole purpose of
> > > drmm_crtc_init_with_planes().
> > > 
> > > We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()", it's wrong.
> > 
> > So we end up with mixed use of managed and unmanaged APIs?
> 
> In this case, yes. However, I don't think this makes it inconsistent. They
> only thing drmm_crtc_init_with_planes() does different than
> drm_crtc_init_with_planes() is that it set's things up to automatically call
> drm_crtc_cleanup() on .destroy. Since this driver also does a register write
> in the .destroy callback and hence we can't get rid of the callback we can
> just keep it as it is.
> 
> > 
> > > 
> > > Do you want me to send a v2 for that?
> > 
> > Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.
> > 
> > BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources,
> > however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
> > hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
> > to me again what are you trying to fix?
> 
> Sure! DRM managed resources are cleaned up whenever the last reference is
> put. This is not necessarily the case when the driver is unbound, hence
> there might still be calls into the driver and therefore we must protect
> resources that are bound to the driver/device lifecycle (e.g. a MMIO region
> mapped via devm_ioremap_resource()) from being accessed. That's why the
> hdlcd_write() and hdlcd_read() calls in the crtc callbacks need to be
> protected.
> 
> However, of course, the changes needed to achieve that should not result
> into hanging rmmod. Unfortunately, just by looking at the patches again I
> don't see how this could happen.
> 
> Do you mind trying again with my v2 (although v2 shouldn't make a difference
> for this issue) and provide the back-trace when it hangs?

Hi Danilo,


I've finally got a replacement Juno board that it is stable enough.

I've tried your v2 on top of 7860d720a84c ("drm/msm: Fix build break with recent mm tree") which
is the head of drm-next today and rmmod hangs. /proc/<pid_of_rmmod>/stack shows:

[<0>] __synchronize_srcu.part.0+0x78/0xec
[<0>] synchronize_srcu+0xe0/0x134
[<0>] drm_dev_unplug+0x2c/0x60 [drm]
[<0>] hdlcd_drm_unbind+0x20/0xc0 [hdlcd]
[<0>] component_master_del+0xa4/0xc0
[<0>] hdlcd_remove+0x1c/0x2c [hdlcd]
[<0>] platform_remove+0x28/0x60
[<0>] device_remove+0x4c/0x80
[<0>] device_release_driver_internal+0x1e4/0x250
[<0>] driver_detach+0x50/0xe0
[<0>] bus_remove_driver+0x5c/0xbc
[<0>] driver_unregister+0x30/0x60
[<0>] platform_driver_unregister+0x14/0x20
[<0>] hdlcd_platform_driver_exit+0x1c/0xe40 [hdlcd]
[<0>] __arm64_sys_delete_module+0x18c/0x240
[<0>] invoke_syscall+0x48/0x114
[<0>] el0_svc_common.constprop.0+0xcc/0xec
[<0>] do_el0_svc+0x2c/0xc0
[<0>] el0_svc+0x2c/0x84
[<0>] el0t_64_sync_handler+0x11c/0x150
[<0>] el0t_64_sync+0x18c/0x190

My quick guess would be that the mixing of managed and unmanaged APIs manages to
confuse the sleepable RCUs and we get the hang. Will chat with Daniel Vetter next
week at XDC on what would be the best approach here.

Best regards,
Liviu




> 
> Thanks,
> Danilo
> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > - Danilo
> > > 
> > > 
> > > 
> > > On 9/12/22 19:36, Liviu Dudau wrote:
> > > > Hi Danilo,
> > > > 
> > > > I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> > > > and on start up I get a warning:
> > > > 
> > > > [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> > > > [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> > > > 
> > > > It looks like the .destroy hook is still required or I'm missing some other required
> > > > series where the WARN has been removed?
> > > > 
> > > > Best regards,
> > > > Liviu
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Liviu Dudau]

On Wed, Sep 14, 2022 at 12:03:58AM +0200, Danilo Krummrich wrote:
> On 9/13/22 10:58, Liviu Dudau wrote:
> > On Mon, Sep 12, 2022 at 09:50:26PM +0200, Danilo Krummrich wrote:
> > > Hi Liviu,
> > 
> > Hi Danilo,
> > 
> > > 
> > > Thanks for having a look!
> > > 
> > > This is not about this patch, it's about patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()".
> > 
> > Agree! However, this is the patch that removes the .destroy hook, so I've replied here.
> 
> This is a different .destroy hook, it's the struct drm_plane_funcs one, not
> the struct drm_crtc_funcs one, which the warning is about. Anyway, as said,
> we can just drop the mentioned patch. :-)
> 
> > 
> > > 
> > > And there it's the other way around. When using drmm_crtc_init_with_planes()
> > > we shouldn't have a destroy hook in place, that's the whole purpose of
> > > drmm_crtc_init_with_planes().
> > > 
> > > We should just drop patch 3/7 "drm/arm/hdlcd: crtc: use
> > > drmm_crtc_init_with_planes()", it's wrong.
> > 
> > So we end up with mixed use of managed and unmanaged APIs?
> 
> In this case, yes. However, I don't think this makes it inconsistent. They
> only thing drmm_crtc_init_with_planes() does different than
> drm_crtc_init_with_planes() is that it set's things up to automatically call
> drm_crtc_cleanup() on .destroy. Since this driver also does a register write
> in the .destroy callback and hence we can't get rid of the callback we can
> just keep it as it is.
> 
> > 
> > > 
> > > Do you want me to send a v2 for that?
> > 
> > Yes please! It would help me to understand your thinking around the whole lifecycle of the driver.
> > 
> > BTW, I appreciate the care in patches 5-7 to make sure that the driver doesn't access freed resources,
> > however I'm not sure I like the fact that rmmod-ing the hdlcd driver while I have an fbcon running
> > hangs now the command and prevents a kernel reboot, while it works without your series. Can you explain
> > to me again what are you trying to fix?
> 
> Sure! DRM managed resources are cleaned up whenever the last reference is
> put. This is not necessarily the case when the driver is unbound, hence
> there might still be calls into the driver and therefore we must protect
> resources that are bound to the driver/device lifecycle (e.g. a MMIO region
> mapped via devm_ioremap_resource()) from being accessed. That's why the
> hdlcd_write() and hdlcd_read() calls in the crtc callbacks need to be
> protected.
> 
> However, of course, the changes needed to achieve that should not result
> into hanging rmmod. Unfortunately, just by looking at the patches again I
> don't see how this could happen.
> 
> Do you mind trying again with my v2 (although v2 shouldn't make a difference
> for this issue) and provide the back-trace when it hangs?

Hi Danilo,


I've finally got a replacement Juno board that it is stable enough.

I've tried your v2 on top of 7860d720a84c ("drm/msm: Fix build break with recent mm tree") which
is the head of drm-next today and rmmod hangs. /proc/<pid_of_rmmod>/stack shows:

[<0>] __synchronize_srcu.part.0+0x78/0xec
[<0>] synchronize_srcu+0xe0/0x134
[<0>] drm_dev_unplug+0x2c/0x60 [drm]
[<0>] hdlcd_drm_unbind+0x20/0xc0 [hdlcd]
[<0>] component_master_del+0xa4/0xc0
[<0>] hdlcd_remove+0x1c/0x2c [hdlcd]
[<0>] platform_remove+0x28/0x60
[<0>] device_remove+0x4c/0x80
[<0>] device_release_driver_internal+0x1e4/0x250
[<0>] driver_detach+0x50/0xe0
[<0>] bus_remove_driver+0x5c/0xbc
[<0>] driver_unregister+0x30/0x60
[<0>] platform_driver_unregister+0x14/0x20
[<0>] hdlcd_platform_driver_exit+0x1c/0xe40 [hdlcd]
[<0>] __arm64_sys_delete_module+0x18c/0x240
[<0>] invoke_syscall+0x48/0x114
[<0>] el0_svc_common.constprop.0+0xcc/0xec
[<0>] do_el0_svc+0x2c/0xc0
[<0>] el0_svc+0x2c/0x84
[<0>] el0t_64_sync_handler+0x11c/0x150
[<0>] el0t_64_sync+0x18c/0x190

My quick guess would be that the mixing of managed and unmanaged APIs manages to
confuse the sleepable RCUs and we get the hang. Will chat with Daniel Vetter next
week at XDC on what would be the best approach here.

Best regards,
Liviu




> 
> Thanks,
> Danilo
> 
> > 
> > Best regards,
> > Liviu
> > 
> > 
> > > 
> > > - Danilo
> > > 
> > > 
> > > 
> > > On 9/12/22 19:36, Liviu Dudau wrote:
> > > > Hi Danilo,
> > > > 
> > > > I have applied your patch series for HDLCD on top of drm-next (commit 213cb76ddc8b)
> > > > and on start up I get a warning:
> > > > 
> > > > [   12.882554] hdlcd 7ff50000.hdlcd: drm_WARN_ON(funcs && funcs->destroy)
> > > > [   12.882596] WARNING: CPU: 1 PID: 211 at drivers/gpu/drm/drm_crtc.c:393 __drmm_crtc_init_with_planes+0x70/0xf0 [drm]
> > > > 
> > > > It looks like the .destroy hook is still required or I'm missing some other required
> > > > series where the WARN has been removed?
> > > > 
> > > > Best regards,
> > > > Liviu
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Hi Liviu,

On 9/30/22 18:56, Liviu Dudau wrote:
> On Wed, Sep 14, 2022 at 12:03:58AM +0200, Danilo Krummrich wrote:
>> Do you mind trying again with my v2 (although v2 shouldn't make a difference
>> for this issue) and provide the back-trace when it hangs?
> 
> Hi Danilo,
> 
> 
> I've finally got a replacement Juno board that it is stable enough.

That's great!

> 
> I've tried your v2 on top of 7860d720a84c ("drm/msm: Fix build break with recent mm tree") which
> is the head of drm-next today and rmmod hangs. /proc/<pid_of_rmmod>/stack shows:

Thanks for taking the time to test the patches and providing this 
stacktrace.

> 
> [<0>] __synchronize_srcu.part.0+0x78/0xec
> [<0>] synchronize_srcu+0xe0/0x134
> [<0>] drm_dev_unplug+0x2c/0x60 [drm]
> [<0>] hdlcd_drm_unbind+0x20/0xc0 [hdlcd]
> [<0>] component_master_del+0xa4/0xc0
> [<0>] hdlcd_remove+0x1c/0x2c [hdlcd]
> [<0>] platform_remove+0x28/0x60
> [<0>] device_remove+0x4c/0x80
> [<0>] device_release_driver_internal+0x1e4/0x250
> [<0>] driver_detach+0x50/0xe0
> [<0>] bus_remove_driver+0x5c/0xbc
> [<0>] driver_unregister+0x30/0x60
> [<0>] platform_driver_unregister+0x14/0x20
> [<0>] hdlcd_platform_driver_exit+0x1c/0xe40 [hdlcd]
> [<0>] __arm64_sys_delete_module+0x18c/0x240
> [<0>] invoke_syscall+0x48/0x114
> [<0>] el0_svc_common.constprop.0+0xcc/0xec
> [<0>] do_el0_svc+0x2c/0xc0
> [<0>] el0_svc+0x2c/0x84
> [<0>] el0t_64_sync_handler+0x11c/0x150
> [<0>] el0t_64_sync+0x18c/0x190
> 

I think I figured it out. I messed up two of the srcu read-side critical 
sections by overlooking alternate return paths within those sections - 
yikes!

I also found a potential use-after-free I accidentally introduced while 
removing the patch in v2.

Finally, I added a patch to remove unnecessary calls to 
drm_mode_config_cleanup() and replaced drm_mode_config_init() with 
drmm_mode_config_init().

I will send you a v3 containing those fixes.

> My quick guess would be that the mixing of managed and unmanaged APIs manages to
> confuse the sleepable RCUs and we get the hang.

I guess you're referring to drm_crtc_init_with_planes() and providing a 
.destroy callback in hdlcd_crtc_funcs. Actually, this .destroy callback 
is handled in the same way as when initializing the crtc with 
drmm_crtc_init_with_planes(), hence I don't think we have a mix of 
managed and unmanaged APIs here.

drmm_crtc_init_with_planes() just calls __drm_crtc_init_with_planes() 
and adds the cleanup via drmm_add_action_or_reset().

Having a .destroy callback registered via drm_crtc_init_with_planes() 
ends up the same way, since drm_mode_config_init() simply calls 
drmm_mode_config_init(), which also registers drm_mode_config_cleanup() 
(ultimately calling the crtc->destroy callback) via 
drmm_add_action_or_reset().

- Danilo

> Will chat with Daniel Vetter next
> week at XDC on what would be the best approach here.
> 
> Best regards,
> Liviu

Re: [PATCH RESEND drm-misc-next 4/7] drm/arm/hdlcd: plane: use drm managed resources

[Danilo Krummrich]

Hi Liviu,

On 9/30/22 18:56, Liviu Dudau wrote:
> On Wed, Sep 14, 2022 at 12:03:58AM +0200, Danilo Krummrich wrote:
>> Do you mind trying again with my v2 (although v2 shouldn't make a difference
>> for this issue) and provide the back-trace when it hangs?
> 
> Hi Danilo,
> 
> 
> I've finally got a replacement Juno board that it is stable enough.

That's great!

> 
> I've tried your v2 on top of 7860d720a84c ("drm/msm: Fix build break with recent mm tree") which
> is the head of drm-next today and rmmod hangs. /proc/<pid_of_rmmod>/stack shows:

Thanks for taking the time to test the patches and providing this 
stacktrace.

> 
> [<0>] __synchronize_srcu.part.0+0x78/0xec
> [<0>] synchronize_srcu+0xe0/0x134
> [<0>] drm_dev_unplug+0x2c/0x60 [drm]
> [<0>] hdlcd_drm_unbind+0x20/0xc0 [hdlcd]
> [<0>] component_master_del+0xa4/0xc0
> [<0>] hdlcd_remove+0x1c/0x2c [hdlcd]
> [<0>] platform_remove+0x28/0x60
> [<0>] device_remove+0x4c/0x80
> [<0>] device_release_driver_internal+0x1e4/0x250
> [<0>] driver_detach+0x50/0xe0
> [<0>] bus_remove_driver+0x5c/0xbc
> [<0>] driver_unregister+0x30/0x60
> [<0>] platform_driver_unregister+0x14/0x20
> [<0>] hdlcd_platform_driver_exit+0x1c/0xe40 [hdlcd]
> [<0>] __arm64_sys_delete_module+0x18c/0x240
> [<0>] invoke_syscall+0x48/0x114
> [<0>] el0_svc_common.constprop.0+0xcc/0xec
> [<0>] do_el0_svc+0x2c/0xc0
> [<0>] el0_svc+0x2c/0x84
> [<0>] el0t_64_sync_handler+0x11c/0x150
> [<0>] el0t_64_sync+0x18c/0x190
> 

I think I figured it out. I messed up two of the srcu read-side critical 
sections by overlooking alternate return paths within those sections - 
yikes!

I also found a potential use-after-free I accidentally introduced while 
removing the patch in v2.

Finally, I added a patch to remove unnecessary calls to 
drm_mode_config_cleanup() and replaced drm_mode_config_init() with 
drmm_mode_config_init().

I will send you a v3 containing those fixes.

> My quick guess would be that the mixing of managed and unmanaged APIs manages to
> confuse the sleepable RCUs and we get the hang.

I guess you're referring to drm_crtc_init_with_planes() and providing a 
.destroy callback in hdlcd_crtc_funcs. Actually, this .destroy callback 
is handled in the same way as when initializing the crtc with 
drmm_crtc_init_with_planes(), hence I don't think we have a mix of 
managed and unmanaged APIs here.

drmm_crtc_init_with_planes() just calls __drm_crtc_init_with_planes() 
and adds the cleanup via drmm_add_action_or_reset().

Having a .destroy callback registered via drm_crtc_init_with_planes() 
ends up the same way, since drm_mode_config_init() simply calls 
drmm_mode_config_init(), which also registers drm_mode_config_cleanup() 
(ultimately calling the crtc->destroy callback) via 
drmm_add_action_or_reset().

- Danilo

> Will chat with Daniel Vetter next
> week at XDC on what would be the best approach here.
> 
> Best regards,
> Liviu