diff mbox series

drm: xlnx: zynqmp_dpsub: Fix missing drm_bridge_add() call

Message ID 20240312-xilinx-dp-lock-fix-v1-1-1698f9f03bac@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series drm: xlnx: zynqmp_dpsub: Fix missing drm_bridge_add() call | expand

Commit Message

Tomi Valkeinen March 12, 2024, 8:51 a.m. UTC 
The driver creates a bridge, but never calls drm_bridge_add() when
non-live input is used. This leaves the bridge's hpd_mutex
uninitialized, leading to:

WARNING: CPU: 0 PID: 9 at kernel/locking/mutex.c:582 __mutex_lock+0x708/0x840

Add the bridge add & remove calls so that the bridge gets managed
correctly.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Fixes: 561671612394 ("drm: xlnx: zynqmp_dpsub: Add support for live video input")
---
 drivers/gpu/drm/xlnx/zynqmp_dp.c    | 4 ++++
 drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)


---
base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
change-id: 20240312-xilinx-dp-lock-fix-cf68f43a7bab

Best regards,

Comments

Laurent Pinchart April 26, 2024, 9:25 a.m. UTC | #1 
Hi Tomi,

Thank you for the patch.

On Tue, Mar 12, 2024 at 10:51:15AM +0200, Tomi Valkeinen wrote:
> The driver creates a bridge, but never calls drm_bridge_add() when
> non-live input is used. This leaves the bridge's hpd_mutex
> uninitialized, leading to:
> 
> WARNING: CPU: 0 PID: 9 at kernel/locking/mutex.c:582 __mutex_lock+0x708/0x840
> 
> Add the bridge add & remove calls so that the bridge gets managed
> correctly.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Fixes: 561671612394 ("drm: xlnx: zynqmp_dpsub: Add support for live video input")
> ---
>  drivers/gpu/drm/xlnx/zynqmp_dp.c    | 4 ++++
>  drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 4 ----
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> index a0606fab0e22..9f750740dfb8 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
> @@ -1761,6 +1761,8 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
>  
>  	dpsub->dp = dp;
>  
> +	drm_bridge_add(dpsub->bridge);
> +

This means that the bridge will be exposed to users before
zynqmp_disp_probe() is called, opening the door to a potential
use-before-init. The risk is mostly theoretical at this point I believe,
but it's still not a direction I'd like to that. Could you call
drm_bridge_add() in zynqmp_dpsub_probe(), between zynqmp_disp_probe()
and zynqmp_dpsub_drm_init() ?

>  	dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
>  		dp->num_lanes);
>  
> @@ -1789,4 +1791,6 @@ void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
>  
>  	zynqmp_dp_phy_exit(dp);
>  	zynqmp_dp_reset(dp, true);
> +
> +	drm_bridge_remove(dpsub->bridge);
>  }
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> index 88eb33acd5f0..3933c4f1a44f 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
> @@ -260,8 +260,6 @@ static int zynqmp_dpsub_probe(struct platform_device *pdev)
>  		ret = zynqmp_dpsub_drm_init(dpsub);
>  		if (ret)
>  			goto err_disp;
> -	} else {
> -		drm_bridge_add(dpsub->bridge);
>  	}
>  
>  	dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
> @@ -288,8 +286,6 @@ static void zynqmp_dpsub_remove(struct platform_device *pdev)
>  
>  	if (dpsub->drm)
>  		zynqmp_dpsub_drm_cleanup(dpsub);
> -	else
> -		drm_bridge_remove(dpsub->bridge);
>  
>  	zynqmp_disp_remove(dpsub);
>  	zynqmp_dp_remove(dpsub);
> 
> ---
> base-commit: e8f897f4afef0031fe618a8e94127a0934896aba
> change-id: 20240312-xilinx-dp-lock-fix-cf68f43a7bab
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index a0606fab0e22..9f750740dfb8 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1761,6 +1761,8 @@  int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
 
 	dpsub->dp = dp;
 
+	drm_bridge_add(dpsub->bridge);
+
 	dev_dbg(dp->dev, "ZynqMP DisplayPort Tx probed with %u lanes\n",
 		dp->num_lanes);
 
@@ -1789,4 +1791,6 @@  void zynqmp_dp_remove(struct zynqmp_dpsub *dpsub)
 
 	zynqmp_dp_phy_exit(dp);
 	zynqmp_dp_reset(dp, true);
+
+	drm_bridge_remove(dpsub->bridge);
 }
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
index 88eb33acd5f0..3933c4f1a44f 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.c
@@ -260,8 +260,6 @@  static int zynqmp_dpsub_probe(struct platform_device *pdev)
 		ret = zynqmp_dpsub_drm_init(dpsub);
 		if (ret)
 			goto err_disp;
-	} else {
-		drm_bridge_add(dpsub->bridge);
 	}
 
 	dev_info(&pdev->dev, "ZynqMP DisplayPort Subsystem driver probed");
@@ -288,8 +286,6 @@  static void zynqmp_dpsub_remove(struct platform_device *pdev)
 
 	if (dpsub->drm)
 		zynqmp_dpsub_drm_cleanup(dpsub);
-	else
-		drm_bridge_remove(dpsub->bridge);
 
 	zynqmp_disp_remove(dpsub);
 	zynqmp_dp_remove(dpsub);