[PATCH 0/3] drm: fix some bridge api misunderstandings

[PATCH 0/3] drm: fix some bridge api misunderstandings

[Peter Rosin]

Hi!

While looking at various drm bridge users, I came across these
issues.

Cheers,
Peter

Peter Rosin (3):
  drm/sti: do not remove the drm_bridge that was never added
  drm/rockchip: lvds: avoid duplicating drm_bridge_attach
  drm/exynos: hdmi: avoid duplicating drm_bridge_attach

 drivers/gpu/drm/exynos/exynos_hdmi.c     | 2 --
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 drivers/gpu/drm/sti/sti_hda.c            | 1 -
 drivers/gpu/drm/sti/sti_hdmi.c           | 1 -
 4 files changed, 6 deletions(-)

-- 
2.11.0

[PATCH 0/3] drm: fix some bridge api misunderstandings

[Peter Rosin]

Hi!

While looking at various drm bridge users, I came across these
issues.

Cheers,
Peter

Peter Rosin (3):
  drm/sti: do not remove the drm_bridge that was never added
  drm/rockchip: lvds: avoid duplicating drm_bridge_attach
  drm/exynos: hdmi: avoid duplicating drm_bridge_attach

 drivers/gpu/drm/exynos/exynos_hdmi.c     | 2 --
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 drivers/gpu/drm/sti/sti_hda.c            | 1 -
 drivers/gpu/drm/sti/sti_hdmi.c           | 1 -
 4 files changed, 6 deletions(-)

-- 
2.11.0

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

The more natural approach would perhaps be to add an drm_bridge_add,
but there are several other bridges that never call drm_bridge_add.
Just removing the drm_bridge_remove is the easier fix.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/sti/sti_hda.c  | 1 -
 drivers/gpu/drm/sti/sti_hdmi.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 67bbdb49fffc..199db13f565c 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_sysfs:
-	drm_bridge_remove(bridge);
 	return -EINVAL;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 58f431102512..932724784942 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_sysfs:
-	drm_bridge_remove(bridge);
 	hdmi->drm_connector = NULL;
 	return -EINVAL;
 }
-- 
2.11.0

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

The more natural approach would perhaps be to add an drm_bridge_add,
but there are several other bridges that never call drm_bridge_add.
Just removing the drm_bridge_remove is the easier fix.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/sti/sti_hda.c  | 1 -
 drivers/gpu/drm/sti/sti_hdmi.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
index 67bbdb49fffc..199db13f565c 100644
--- a/drivers/gpu/drm/sti/sti_hda.c
+++ b/drivers/gpu/drm/sti/sti_hda.c
@@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_sysfs:
-	drm_bridge_remove(bridge);
 	return -EINVAL;
 }
 
diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 58f431102512..932724784942 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
 	return 0;
 
 err_sysfs:
-	drm_bridge_remove(bridge);
 	hdmi->drm_connector = NULL;
 	return -EINVAL;
 }
-- 
2.11.0

[PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..4bd94b167d2c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -446,14 +446,12 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 			goto err_free_connector;
 		}
 	} else {
-		lvds->bridge->encoder = encoder;
 		ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
 		if (ret) {
 			DRM_DEV_ERROR(drm_dev->dev,
 				      "failed to attach bridge: %d\n", ret);
 			goto err_free_encoder;
 		}
-		encoder->bridge = lvds->bridge;
 	}
 
 	pm_runtime_enable(dev);
-- 
2.11.0

[PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index e67f4ea28c0e..4bd94b167d2c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -446,14 +446,12 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 			goto err_free_connector;
 		}
 	} else {
-		lvds->bridge->encoder = encoder;
 		ret = drm_bridge_attach(encoder, lvds->bridge, NULL);
 		if (ret) {
 			DRM_DEV_ERROR(drm_dev->dev,
 				      "failed to attach bridge: %d\n", ret);
 			goto err_free_encoder;
 		}
-		encoder->bridge = lvds->bridge;
 	}
 
 	pm_runtime_enable(dev);
-- 
2.11.0

[PATCH 3/3] drm/exynos: hdmi: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index abd84cbcf1c2..09c4bc0b1859 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -954,8 +954,6 @@ static int hdmi_create_connector(struct drm_encoder *encoder)
 	drm_mode_connector_attach_encoder(connector, encoder);
 
 	if (hdata->bridge) {
-		encoder->bridge = hdata->bridge;
-		hdata->bridge->encoder = encoder;
 		ret = drm_bridge_attach(encoder, hdata->bridge, NULL);
 		if (ret)
 			DRM_ERROR("Failed to attach bridge\n");
-- 
2.11.0

[PATCH 3/3] drm/exynos: hdmi: avoid duplicating drm_bridge_attach

[Peter Rosin]

drm_bridge_attach takes care of these assignments, so there is no need
to open-code them a second time.

Signed-off-by: Peter Rosin <peda@axentia.se>
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index abd84cbcf1c2..09c4bc0b1859 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -954,8 +954,6 @@ static int hdmi_create_connector(struct drm_encoder *encoder)
 	drm_mode_connector_attach_encoder(connector, encoder);
 
 	if (hdata->bridge) {
-		encoder->bridge = hdata->bridge;
-		hdata->bridge->encoder = encoder;
 		ret = drm_bridge_attach(encoder, hdata->bridge, NULL);
 		if (ret)
 			DRM_ERROR("Failed to attach bridge\n");
-- 
2.11.0

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> The more natural approach would perhaps be to add an drm_bridge_add,
> but there are several other bridges that never call drm_bridge_add.
> Just removing the drm_bridge_remove is the easier fix.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

This mess is much bigger. There's 2 pairs of bridge functions:

- drm_bridge_attach/detach. Those are meant to be called by the overall
  drm driver to connect/disconnect a drm_bridge.

- drm_bridge_add/remove. These are supposed to be called by the bridge
  driver itself to register/unregister itself. Maybe we should rename
  them, since the same issue happens with drm_panel, with the same
  confusion.

I thought someone was working on a cleanup series to fix this mess, but I
didn't find anything.
-Daniel

> ---
>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> index 67bbdb49fffc..199db13f565c 100644
> --- a/drivers/gpu/drm/sti/sti_hda.c
> +++ b/drivers/gpu/drm/sti/sti_hda.c
> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	return -EINVAL;
>  }
>  
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index 58f431102512..932724784942 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	hdmi->drm_connector = NULL;
>  	return -EINVAL;
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> The more natural approach would perhaps be to add an drm_bridge_add,
> but there are several other bridges that never call drm_bridge_add.
> Just removing the drm_bridge_remove is the easier fix.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

This mess is much bigger. There's 2 pairs of bridge functions:

- drm_bridge_attach/detach. Those are meant to be called by the overall
  drm driver to connect/disconnect a drm_bridge.

- drm_bridge_add/remove. These are supposed to be called by the bridge
  driver itself to register/unregister itself. Maybe we should rename
  them, since the same issue happens with drm_panel, with the same
  confusion.

I thought someone was working on a cleanup series to fix this mess, but I
didn't find anything.
-Daniel

> ---
>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> index 67bbdb49fffc..199db13f565c 100644
> --- a/drivers/gpu/drm/sti/sti_hda.c
> +++ b/drivers/gpu/drm/sti/sti_hda.c
> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	return -EINVAL;
>  }
>  
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index 58f431102512..932724784942 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	hdmi->drm_connector = NULL;
>  	return -EINVAL;
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> The more natural approach would perhaps be to add an drm_bridge_add,
> but there are several other bridges that never call drm_bridge_add.
> Just removing the drm_bridge_remove is the easier fix.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

This mess is much bigger. There's 2 pairs of bridge functions:

- drm_bridge_attach/detach. Those are meant to be called by the overall
  drm driver to connect/disconnect a drm_bridge.

- drm_bridge_add/remove. These are supposed to be called by the bridge
  driver itself to register/unregister itself. Maybe we should rename
  them, since the same issue happens with drm_panel, with the same
  confusion.

I thought someone was working on a cleanup series to fix this mess, but I
didn't find anything.
-Daniel

> ---
>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> index 67bbdb49fffc..199db13f565c 100644
> --- a/drivers/gpu/drm/sti/sti_hda.c
> +++ b/drivers/gpu/drm/sti/sti_hda.c
> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	return -EINVAL;
>  }
>  
> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> index 58f431102512..932724784942 100644
> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>  	return 0;
>  
>  err_sysfs:
> -	drm_bridge_remove(bridge);
>  	hdmi->drm_connector = NULL;
>  	return -EINVAL;
>  }
> -- 
> 2.11.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-03 11:06, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>> The more natural approach would perhaps be to add an drm_bridge_add,
>> but there are several other bridges that never call drm_bridge_add.
>> Just removing the drm_bridge_remove is the easier fix.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> This mess is much bigger. There's 2 pairs of bridge functions:
> 
> - drm_bridge_attach/detach. Those are meant to be called by the overall
>   drm driver to connect/disconnect a drm_bridge.
> 
> - drm_bridge_add/remove. These are supposed to be called by the bridge
>   driver itself to register/unregister itself. Maybe we should rename
>   them, since the same issue happens with drm_panel, with the same
>   confusion.
> 
> I thought someone was working on a cleanup series to fix this mess, but I
> didn't find anything.

Ok, I just spotted the imbalance and didn't really dig into what
actually happens in these error paths. Now that I have done so I
believe that the removed drm_bridge_remove calls causes NULL
dereferences if/when the error paths are triggered.

So, I don't think this can wait for some bigger cleanup.

drm_bridge_remove calls list_del_init calls __list_del_entry calls
__list_del with NULL in both prev and next since the list member
is never initialized. prev and next are dereferenced by __list_del
and you have *boom*

I recommend adding the tag

Fixes: 84601dbdea36 ("drm: sti: rework init sequence")

so that stable picks this one up.

Cheers,
Peter

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>> index 67bbdb49fffc..199db13f565c 100644
>> --- a/drivers/gpu/drm/sti/sti_hda.c
>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>>  	return 0;
>>  
>>  err_sysfs:
>> -	drm_bridge_remove(bridge);
>>  	return -EINVAL;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 58f431102512..932724784942 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>>  	return 0;
>>  
>>  err_sysfs:
>> -	drm_bridge_remove(bridge);
>>  	hdmi->drm_connector = NULL;
>>  	return -EINVAL;
>>  }
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-03 11:06, Daniel Vetter wrote:
> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>> The more natural approach would perhaps be to add an drm_bridge_add,
>> but there are several other bridges that never call drm_bridge_add.
>> Just removing the drm_bridge_remove is the easier fix.
>>
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> This mess is much bigger. There's 2 pairs of bridge functions:
> 
> - drm_bridge_attach/detach. Those are meant to be called by the overall
>   drm driver to connect/disconnect a drm_bridge.
> 
> - drm_bridge_add/remove. These are supposed to be called by the bridge
>   driver itself to register/unregister itself. Maybe we should rename
>   them, since the same issue happens with drm_panel, with the same
>   confusion.
> 
> I thought someone was working on a cleanup series to fix this mess, but I
> didn't find anything.

Ok, I just spotted the imbalance and didn't really dig into what
actually happens in these error paths. Now that I have done so I
believe that the removed drm_bridge_remove calls causes NULL
dereferences if/when the error paths are triggered.

So, I don't think this can wait for some bigger cleanup.

drm_bridge_remove calls list_del_init calls __list_del_entry calls
__list_del with NULL in both prev and next since the list member
is never initialized. prev and next are dereferenced by __list_del
and you have *boom*

I recommend adding the tag

Fixes: 84601dbdea36 ("drm: sti: rework init sequence")

so that stable picks this one up.

Cheers,
Peter

> -Daniel
> 
>> ---
>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>  2 files changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>> index 67bbdb49fffc..199db13f565c 100644
>> --- a/drivers/gpu/drm/sti/sti_hda.c
>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>>  	return 0;
>>  
>>  err_sysfs:
>> -	drm_bridge_remove(bridge);
>>  	return -EINVAL;
>>  }
>>  
>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>> index 58f431102512..932724784942 100644
>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>>  	return 0;
>>  
>>  err_sysfs:
>> -	drm_bridge_remove(bridge);
>>  	hdmi->drm_connector = NULL;
>>  	return -EINVAL;
>>  }
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> On 2018-05-03 11:06, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >> The more natural approach would perhaps be to add an drm_bridge_add,
> >> but there are several other bridges that never call drm_bridge_add.
> >> Just removing the drm_bridge_remove is the easier fix.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> > 
> > This mess is much bigger. There's 2 pairs of bridge functions:
> > 
> > - drm_bridge_attach/detach. Those are meant to be called by the overall
> >   drm driver to connect/disconnect a drm_bridge.
> > 
> > - drm_bridge_add/remove. These are supposed to be called by the bridge
> >   driver itself to register/unregister itself. Maybe we should rename
> >   them, since the same issue happens with drm_panel, with the same
> >   confusion.
> > 
> > I thought someone was working on a cleanup series to fix this mess, but I
> > didn't find anything.
> 
> Ok, I just spotted the imbalance and didn't really dig into what
> actually happens in these error paths. Now that I have done so I
> believe that the removed drm_bridge_remove calls causes NULL
> dereferences if/when the error paths are triggered.
> 
> So, I don't think this can wait for some bigger cleanup.
> 
> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> __list_del with NULL in both prev and next since the list member
> is never initialized. prev and next are dereferenced by __list_del
> and you have *boom*
> 
> I recommend adding the tag
> 
> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> 
> so that stable picks this one up.

I just wanted to correct your commit message text - the correct solution
is definitely _not_ for sti here to call drm_bridge_add. It should call
drm_bridge_attach/detach only, as a pair.

I didn't check whether you instead have a _detach call missing or what's
going on here.
-Daniel
> 
> Cheers,
> Peter
> 
> > -Daniel
> > 
> >> ---
> >>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>  2 files changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >> index 67bbdb49fffc..199db13f565c 100644
> >> --- a/drivers/gpu/drm/sti/sti_hda.c
> >> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	return -EINVAL;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >> index 58f431102512..932724784942 100644
> >> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	hdmi->drm_connector = NULL;
> >>  	return -EINVAL;
> >>  }
> >> -- 
> >> 2.11.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> On 2018-05-03 11:06, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >> The more natural approach would perhaps be to add an drm_bridge_add,
> >> but there are several other bridges that never call drm_bridge_add.
> >> Just removing the drm_bridge_remove is the easier fix.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> > 
> > This mess is much bigger. There's 2 pairs of bridge functions:
> > 
> > - drm_bridge_attach/detach. Those are meant to be called by the overall
> >   drm driver to connect/disconnect a drm_bridge.
> > 
> > - drm_bridge_add/remove. These are supposed to be called by the bridge
> >   driver itself to register/unregister itself. Maybe we should rename
> >   them, since the same issue happens with drm_panel, with the same
> >   confusion.
> > 
> > I thought someone was working on a cleanup series to fix this mess, but I
> > didn't find anything.
> 
> Ok, I just spotted the imbalance and didn't really dig into what
> actually happens in these error paths. Now that I have done so I
> believe that the removed drm_bridge_remove calls causes NULL
> dereferences if/when the error paths are triggered.
> 
> So, I don't think this can wait for some bigger cleanup.
> 
> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> __list_del with NULL in both prev and next since the list member
> is never initialized. prev and next are dereferenced by __list_del
> and you have *boom*
> 
> I recommend adding the tag
> 
> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> 
> so that stable picks this one up.

I just wanted to correct your commit message text - the correct solution
is definitely _not_ for sti here to call drm_bridge_add. It should call
drm_bridge_attach/detach only, as a pair.

I didn't check whether you instead have a _detach call missing or what's
going on here.
-Daniel
> 
> Cheers,
> Peter
> 
> > -Daniel
> > 
> >> ---
> >>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>  2 files changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >> index 67bbdb49fffc..199db13f565c 100644
> >> --- a/drivers/gpu/drm/sti/sti_hda.c
> >> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	return -EINVAL;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >> index 58f431102512..932724784942 100644
> >> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	hdmi->drm_connector = NULL;
> >>  	return -EINVAL;
> >>  }
> >> -- 
> >> 2.11.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> On 2018-05-03 11:06, Daniel Vetter wrote:
> > On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >> The more natural approach would perhaps be to add an drm_bridge_add,
> >> but there are several other bridges that never call drm_bridge_add.
> >> Just removing the drm_bridge_remove is the easier fix.
> >>
> >> Signed-off-by: Peter Rosin <peda@axentia.se>
> > 
> > This mess is much bigger. There's 2 pairs of bridge functions:
> > 
> > - drm_bridge_attach/detach. Those are meant to be called by the overall
> >   drm driver to connect/disconnect a drm_bridge.
> > 
> > - drm_bridge_add/remove. These are supposed to be called by the bridge
> >   driver itself to register/unregister itself. Maybe we should rename
> >   them, since the same issue happens with drm_panel, with the same
> >   confusion.
> > 
> > I thought someone was working on a cleanup series to fix this mess, but I
> > didn't find anything.
> 
> Ok, I just spotted the imbalance and didn't really dig into what
> actually happens in these error paths. Now that I have done so I
> believe that the removed drm_bridge_remove calls causes NULL
> dereferences if/when the error paths are triggered.
> 
> So, I don't think this can wait for some bigger cleanup.
> 
> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> __list_del with NULL in both prev and next since the list member
> is never initialized. prev and next are dereferenced by __list_del
> and you have *boom*
> 
> I recommend adding the tag
> 
> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> 
> so that stable picks this one up.

I just wanted to correct your commit message text - the correct solution
is definitely _not_ for sti here to call drm_bridge_add. It should call
drm_bridge_attach/detach only, as a pair.

I didn't check whether you instead have a _detach call missing or what's
going on here.
-Daniel
> 
> Cheers,
> Peter
> 
> > -Daniel
> > 
> >> ---
> >>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>  2 files changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >> index 67bbdb49fffc..199db13f565c 100644
> >> --- a/drivers/gpu/drm/sti/sti_hda.c
> >> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	return -EINVAL;
> >>  }
> >>  
> >> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >> index 58f431102512..932724784942 100644
> >> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>  	return 0;
> >>  
> >>  err_sysfs:
> >> -	drm_bridge_remove(bridge);
> >>  	hdmi->drm_connector = NULL;
> >>  	return -EINVAL;
> >>  }
> >> -- 
> >> 2.11.0
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:39, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>> but there are several other bridges that never call drm_bridge_add.
>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>
>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>
>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>   drm driver to connect/disconnect a drm_bridge.
>>>
>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>   driver itself to register/unregister itself. Maybe we should rename
>>>   them, since the same issue happens with drm_panel, with the same
>>>   confusion.
>>>
>>> I thought someone was working on a cleanup series to fix this mess, but I
>>> didn't find anything.
>>
>> Ok, I just spotted the imbalance and didn't really dig into what
>> actually happens in these error paths. Now that I have done so I
>> believe that the removed drm_bridge_remove calls causes NULL
>> dereferences if/when the error paths are triggered.
>>
>> So, I don't think this can wait for some bigger cleanup.
>>
>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>> __list_del with NULL in both prev and next since the list member
>> is never initialized. prev and next are dereferenced by __list_del
>> and you have *boom*
>>
>> I recommend adding the tag
>>
>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>
>> so that stable picks this one up.
> 
> I just wanted to correct your commit message text - the correct solution
> is definitely _not_ for sti here to call drm_bridge_add.

Ah, I see what you mean. Do you want me to respin?

>                                                          It should call
> drm_bridge_attach/detach only, as a pair.

Alas, the attach/detach functions are generally not called from the same
level. After the bridge has been attached to an encoder, it is detached
in the generic code shutting down the encoder, i.e. the bridge consumer
is not explicitly involved with bridge detaching.

> I didn't check whether you instead have a _detach call missing or what's
> going on here.

So, even though there is no _detach call, it is still not "missing" as
it is not supposed to be there...

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>>> -Daniel
>>>
>>>> ---
>>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>>>  2 files changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>>>> index 67bbdb49fffc..199db13f565c 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hda.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>>>>  	return 0;
>>>>  
>>>>  err_sysfs:
>>>> -	drm_bridge_remove(bridge);
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> index 58f431102512..932724784942 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>>>>  	return 0;
>>>>  
>>>>  err_sysfs:
>>>> -	drm_bridge_remove(bridge);
>>>>  	hdmi->drm_connector = NULL;
>>>>  	return -EINVAL;
>>>>  }
>>>> -- 
>>>> 2.11.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:39, Daniel Vetter wrote:
> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>> but there are several other bridges that never call drm_bridge_add.
>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>
>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>
>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>
>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>   drm driver to connect/disconnect a drm_bridge.
>>>
>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>   driver itself to register/unregister itself. Maybe we should rename
>>>   them, since the same issue happens with drm_panel, with the same
>>>   confusion.
>>>
>>> I thought someone was working on a cleanup series to fix this mess, but I
>>> didn't find anything.
>>
>> Ok, I just spotted the imbalance and didn't really dig into what
>> actually happens in these error paths. Now that I have done so I
>> believe that the removed drm_bridge_remove calls causes NULL
>> dereferences if/when the error paths are triggered.
>>
>> So, I don't think this can wait for some bigger cleanup.
>>
>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>> __list_del with NULL in both prev and next since the list member
>> is never initialized. prev and next are dereferenced by __list_del
>> and you have *boom*
>>
>> I recommend adding the tag
>>
>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>
>> so that stable picks this one up.
> 
> I just wanted to correct your commit message text - the correct solution
> is definitely _not_ for sti here to call drm_bridge_add.

Ah, I see what you mean. Do you want me to respin?

>                                                          It should call
> drm_bridge_attach/detach only, as a pair.

Alas, the attach/detach functions are generally not called from the same
level. After the bridge has been attached to an encoder, it is detached
in the generic code shutting down the encoder, i.e. the bridge consumer
is not explicitly involved with bridge detaching.

> I didn't check whether you instead have a _detach call missing or what's
> going on here.

So, even though there is no _detach call, it is still not "missing" as
it is not supposed to be there...

Cheers,
Peter

> -Daniel
>>
>> Cheers,
>> Peter
>>
>>> -Daniel
>>>
>>>> ---
>>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
>>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
>>>>  2 files changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
>>>> index 67bbdb49fffc..199db13f565c 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hda.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
>>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
>>>>  	return 0;
>>>>  
>>>>  err_sysfs:
>>>> -	drm_bridge_remove(bridge);
>>>>  	return -EINVAL;
>>>>  }
>>>>  
>>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> index 58f431102512..932724784942 100644
>>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
>>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
>>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
>>>>  	return 0;
>>>>  
>>>>  err_sysfs:
>>>> -	drm_bridge_remove(bridge);
>>>>  	hdmi->drm_connector = NULL;
>>>>  	return -EINVAL;
>>>>  }
>>>> -- 
>>>> 2.11.0
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:59, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
>> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>>> but there are several other bridges that never call drm_bridge_add.
>>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>
>>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>>
>>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>>   drm driver to connect/disconnect a drm_bridge.
>>>>
>>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>>   driver itself to register/unregister itself. Maybe we should rename
>>>>   them, since the same issue happens with drm_panel, with the same
>>>>   confusion.
>>>>
>>>> I thought someone was working on a cleanup series to fix this mess, but I
>>>> didn't find anything.
>>>
>>> Ok, I just spotted the imbalance and didn't really dig into what
>>> actually happens in these error paths. Now that I have done so I
>>> believe that the removed drm_bridge_remove calls causes NULL
>>> dereferences if/when the error paths are triggered.
>>>
>>> So, I don't think this can wait for some bigger cleanup.
>>>
>>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>>> __list_del with NULL in both prev and next since the list member
>>> is never initialized. prev and next are dereferenced by __list_del
>>> and you have *boom*
>>>
>>> I recommend adding the tag
>>>
>>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>>
>>> so that stable picks this one up.
>>
>> I just wanted to correct your commit message text - the correct solution
>> is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?

Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
internal use. It does not drm_bridge_add it, because all that ever does
is adding the bridge to the global lost of bridges. But since this is
a bridge for internal use, there is little point in calling drm_bridge_add,
the driver currently gains nothing by doing so.

But, drm_bridge_add might be a good place to put common stuff for every
bridge in the system, so it might be worthwhile to start requiring all
bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
the sti-hda driver call drm_bridge_add on the bridge it creates.

Do you really think it is actively wrong to call drm_bridge_add for
internal bridges such as this?

Cheers,
Peter

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Peter Rosin]

On 2018-05-07 15:59, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
>> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
>>> On 2018-05-03 11:06, Daniel Vetter wrote:
>>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
>>>>> The more natural approach would perhaps be to add an drm_bridge_add,
>>>>> but there are several other bridges that never call drm_bridge_add.
>>>>> Just removing the drm_bridge_remove is the easier fix.
>>>>>
>>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
>>>>
>>>> This mess is much bigger. There's 2 pairs of bridge functions:
>>>>
>>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
>>>>   drm driver to connect/disconnect a drm_bridge.
>>>>
>>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
>>>>   driver itself to register/unregister itself. Maybe we should rename
>>>>   them, since the same issue happens with drm_panel, with the same
>>>>   confusion.
>>>>
>>>> I thought someone was working on a cleanup series to fix this mess, but I
>>>> didn't find anything.
>>>
>>> Ok, I just spotted the imbalance and didn't really dig into what
>>> actually happens in these error paths. Now that I have done so I
>>> believe that the removed drm_bridge_remove calls causes NULL
>>> dereferences if/when the error paths are triggered.
>>>
>>> So, I don't think this can wait for some bigger cleanup.
>>>
>>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
>>> __list_del with NULL in both prev and next since the list member
>>> is never initialized. prev and next are dereferenced by __list_del
>>> and you have *boom*
>>>
>>> I recommend adding the tag
>>>
>>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
>>>
>>> so that stable picks this one up.
>>
>> I just wanted to correct your commit message text - the correct solution
>> is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?

Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
internal use. It does not drm_bridge_add it, because all that ever does
is adding the bridge to the global lost of bridges. But since this is
a bridge for internal use, there is little point in calling drm_bridge_add,
the driver currently gains nothing by doing so.

But, drm_bridge_add might be a good place to put common stuff for every
bridge in the system, so it might be worthwhile to start requiring all
bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
the sti-hda driver call drm_bridge_add on the bridge it creates.

Do you really think it is actively wrong to call drm_bridge_add for
internal bridges such as this?

Cheers,
Peter

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Mon, May 07, 2018 at 03:59:04PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
> > On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>> but there are several other bridges that never call drm_bridge_add.
> >>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>
> >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>
> >>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>
> >>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>   drm driver to connect/disconnect a drm_bridge.
> >>>
> >>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>   driver itself to register/unregister itself. Maybe we should rename
> >>>   them, since the same issue happens with drm_panel, with the same
> >>>   confusion.
> >>>
> >>> I thought someone was working on a cleanup series to fix this mess, but I
> >>> didn't find anything.
> >>
> >> Ok, I just spotted the imbalance and didn't really dig into what
> >> actually happens in these error paths. Now that I have done so I
> >> believe that the removed drm_bridge_remove calls causes NULL
> >> dereferences if/when the error paths are triggered.
> >>
> >> So, I don't think this can wait for some bigger cleanup.
> >>
> >> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >> __list_del with NULL in both prev and next since the list member
> >> is never initialized. prev and next are dereferenced by __list_del
> >> and you have *boom*
> >>
> >> I recommend adding the tag
> >>
> >> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>
> >> so that stable picks this one up.
> > 
> > I just wanted to correct your commit message text - the correct solution
> > is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?
> 
> >                                                          It should call
> > drm_bridge_attach/detach only, as a pair.
> 
> Alas, the attach/detach functions are generally not called from the same
> level. After the bridge has been attached to an encoder, it is detached
> in the generic code shutting down the encoder, i.e. the bridge consumer
> is not explicitly involved with bridge detaching.
> 
> > I didn't check whether you instead have a _detach call missing or what's
> > going on here.
> 
> So, even though there is no _detach call, it is still not "missing" as
> it is not supposed to be there...

Oh, TIL. Totally missed that we've improved this to be closer to dwim()
semantics. I think your patch is correct as-is and has my

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

It'd be great to improve the kerneldoc for drm_bridge_attach though to
mention that bridges get auto-detached on encoder cleanup as don in
drm_encoder_cleanup(). Care to do that?

And on that note I've again realized that most drivers totally get this
wrong when they set their ->destroy callback to drm_encoder_cleanup
(similar for other kms objects), because that one does _not_ do the final
kfree. Oh well.
-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >>
> >> Cheers,
> >> Peter
> >>
> >>> -Daniel
> >>>
> >>>> ---
> >>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>>>  2 files changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >>>> index 67bbdb49fffc..199db13f565c 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hda.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	return -EINVAL;
> >>>>  }
> >>>>  
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> index 58f431102512..932724784942 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	hdmi->drm_connector = NULL;
> >>>>  	return -EINVAL;
> >>>>  }
> >>>> -- 
> >>>> 2.11.0
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Mon, May 07, 2018 at 03:59:04PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
> > On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>> but there are several other bridges that never call drm_bridge_add.
> >>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>
> >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>
> >>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>
> >>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>   drm driver to connect/disconnect a drm_bridge.
> >>>
> >>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>   driver itself to register/unregister itself. Maybe we should rename
> >>>   them, since the same issue happens with drm_panel, with the same
> >>>   confusion.
> >>>
> >>> I thought someone was working on a cleanup series to fix this mess, but I
> >>> didn't find anything.
> >>
> >> Ok, I just spotted the imbalance and didn't really dig into what
> >> actually happens in these error paths. Now that I have done so I
> >> believe that the removed drm_bridge_remove calls causes NULL
> >> dereferences if/when the error paths are triggered.
> >>
> >> So, I don't think this can wait for some bigger cleanup.
> >>
> >> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >> __list_del with NULL in both prev and next since the list member
> >> is never initialized. prev and next are dereferenced by __list_del
> >> and you have *boom*
> >>
> >> I recommend adding the tag
> >>
> >> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>
> >> so that stable picks this one up.
> > 
> > I just wanted to correct your commit message text - the correct solution
> > is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?
> 
> >                                                          It should call
> > drm_bridge_attach/detach only, as a pair.
> 
> Alas, the attach/detach functions are generally not called from the same
> level. After the bridge has been attached to an encoder, it is detached
> in the generic code shutting down the encoder, i.e. the bridge consumer
> is not explicitly involved with bridge detaching.
> 
> > I didn't check whether you instead have a _detach call missing or what's
> > going on here.
> 
> So, even though there is no _detach call, it is still not "missing" as
> it is not supposed to be there...

Oh, TIL. Totally missed that we've improved this to be closer to dwim()
semantics. I think your patch is correct as-is and has my

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

It'd be great to improve the kerneldoc for drm_bridge_attach though to
mention that bridges get auto-detached on encoder cleanup as don in
drm_encoder_cleanup(). Care to do that?

And on that note I've again realized that most drivers totally get this
wrong when they set their ->destroy callback to drm_encoder_cleanup
(similar for other kms objects), because that one does _not_ do the final
kfree. Oh well.
-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >>
> >> Cheers,
> >> Peter
> >>
> >>> -Daniel
> >>>
> >>>> ---
> >>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>>>  2 files changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >>>> index 67bbdb49fffc..199db13f565c 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hda.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	return -EINVAL;
> >>>>  }
> >>>>  
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> index 58f431102512..932724784942 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	hdmi->drm_connector = NULL;
> >>>>  	return -EINVAL;
> >>>>  }
> >>>> -- 
> >>>> 2.11.0
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Mon, May 07, 2018 at 03:59:04PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:39, Daniel Vetter wrote:
> > On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>> but there are several other bridges that never call drm_bridge_add.
> >>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>
> >>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>
> >>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>
> >>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>   drm driver to connect/disconnect a drm_bridge.
> >>>
> >>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>   driver itself to register/unregister itself. Maybe we should rename
> >>>   them, since the same issue happens with drm_panel, with the same
> >>>   confusion.
> >>>
> >>> I thought someone was working on a cleanup series to fix this mess, but I
> >>> didn't find anything.
> >>
> >> Ok, I just spotted the imbalance and didn't really dig into what
> >> actually happens in these error paths. Now that I have done so I
> >> believe that the removed drm_bridge_remove calls causes NULL
> >> dereferences if/when the error paths are triggered.
> >>
> >> So, I don't think this can wait for some bigger cleanup.
> >>
> >> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >> __list_del with NULL in both prev and next since the list member
> >> is never initialized. prev and next are dereferenced by __list_del
> >> and you have *boom*
> >>
> >> I recommend adding the tag
> >>
> >> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>
> >> so that stable picks this one up.
> > 
> > I just wanted to correct your commit message text - the correct solution
> > is definitely _not_ for sti here to call drm_bridge_add.
> 
> Ah, I see what you mean. Do you want me to respin?
> 
> >                                                          It should call
> > drm_bridge_attach/detach only, as a pair.
> 
> Alas, the attach/detach functions are generally not called from the same
> level. After the bridge has been attached to an encoder, it is detached
> in the generic code shutting down the encoder, i.e. the bridge consumer
> is not explicitly involved with bridge detaching.
> 
> > I didn't check whether you instead have a _detach call missing or what's
> > going on here.
> 
> So, even though there is no _detach call, it is still not "missing" as
> it is not supposed to be there...

Oh, TIL. Totally missed that we've improved this to be closer to dwim()
semantics. I think your patch is correct as-is and has my

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

It'd be great to improve the kerneldoc for drm_bridge_attach though to
mention that bridges get auto-detached on encoder cleanup as don in
drm_encoder_cleanup(). Care to do that?

And on that note I've again realized that most drivers totally get this
wrong when they set their ->destroy callback to drm_encoder_cleanup
(similar for other kms objects), because that one does _not_ do the final
kfree. Oh well.
-Daniel

> 
> Cheers,
> Peter
> 
> > -Daniel
> >>
> >> Cheers,
> >> Peter
> >>
> >>> -Daniel
> >>>
> >>>> ---
> >>>>  drivers/gpu/drm/sti/sti_hda.c  | 1 -
> >>>>  drivers/gpu/drm/sti/sti_hdmi.c | 1 -
> >>>>  2 files changed, 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hda.c b/drivers/gpu/drm/sti/sti_hda.c
> >>>> index 67bbdb49fffc..199db13f565c 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hda.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hda.c
> >>>> @@ -721,7 +721,6 @@ static int sti_hda_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	return -EINVAL;
> >>>>  }
> >>>>  
> >>>> diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> index 58f431102512..932724784942 100644
> >>>> --- a/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> +++ b/drivers/gpu/drm/sti/sti_hdmi.c
> >>>> @@ -1315,7 +1315,6 @@ static int sti_hdmi_bind(struct device *dev, struct device *master, void *data)
> >>>>  	return 0;
> >>>>  
> >>>>  err_sysfs:
> >>>> -	drm_bridge_remove(bridge);
> >>>>  	hdmi->drm_connector = NULL;
> >>>>  	return -EINVAL;
> >>>>  }
> >>>> -- 
> >>>> 2.11.0
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel at lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Mon, May 07, 2018 at 04:24:43PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:59, Peter Rosin wrote:
> > On 2018-05-07 15:39, Daniel Vetter wrote:
> >> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >>> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>>> but there are several other bridges that never call drm_bridge_add.
> >>>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>>
> >>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>>
> >>>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>>
> >>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>>   drm driver to connect/disconnect a drm_bridge.
> >>>>
> >>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>>   driver itself to register/unregister itself. Maybe we should rename
> >>>>   them, since the same issue happens with drm_panel, with the same
> >>>>   confusion.
> >>>>
> >>>> I thought someone was working on a cleanup series to fix this mess, but I
> >>>> didn't find anything.
> >>>
> >>> Ok, I just spotted the imbalance and didn't really dig into what
> >>> actually happens in these error paths. Now that I have done so I
> >>> believe that the removed drm_bridge_remove calls causes NULL
> >>> dereferences if/when the error paths are triggered.
> >>>
> >>> So, I don't think this can wait for some bigger cleanup.
> >>>
> >>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >>> __list_del with NULL in both prev and next since the list member
> >>> is never initialized. prev and next are dereferenced by __list_del
> >>> and you have *boom*
> >>>
> >>> I recommend adding the tag
> >>>
> >>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>>
> >>> so that stable picks this one up.
> >>
> >> I just wanted to correct your commit message text - the correct solution
> >> is definitely _not_ for sti here to call drm_bridge_add.
> > 
> > Ah, I see what you mean. Do you want me to respin?
> 
> Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
> internal use. It does not drm_bridge_add it, because all that ever does
> is adding the bridge to the global lost of bridges. But since this is
> a bridge for internal use, there is little point in calling drm_bridge_add,
> the driver currently gains nothing by doing so.
> 
> But, drm_bridge_add might be a good place to put common stuff for every
> bridge in the system, so it might be worthwhile to start requiring all
> bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
> the sti-hda driver call drm_bridge_add on the bridge it creates.
> 
> Do you really think it is actively wrong to call drm_bridge_add for
> internal bridges such as this?

If we want to share bridge init code, then I think we need a
drm_bridge_init(). Not overload drm_bridge_add (which really should be
drm_bridge_register I think, but oh well, it's at least consistent with
drm_panel_add).
-Daniel

> 
> Cheers,
> Peter
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH 1/3] drm/sti: do not remove the drm_bridge that was never added

[Daniel Vetter]

On Mon, May 07, 2018 at 04:24:43PM +0200, Peter Rosin wrote:
> On 2018-05-07 15:59, Peter Rosin wrote:
> > On 2018-05-07 15:39, Daniel Vetter wrote:
> >> On Thu, May 03, 2018 at 11:12:21PM +0200, Peter Rosin wrote:
> >>> On 2018-05-03 11:06, Daniel Vetter wrote:
> >>>> On Wed, May 02, 2018 at 09:40:23AM +0200, Peter Rosin wrote:
> >>>>> The more natural approach would perhaps be to add an drm_bridge_add,
> >>>>> but there are several other bridges that never call drm_bridge_add.
> >>>>> Just removing the drm_bridge_remove is the easier fix.
> >>>>>
> >>>>> Signed-off-by: Peter Rosin <peda@axentia.se>
> >>>>
> >>>> This mess is much bigger. There's 2 pairs of bridge functions:
> >>>>
> >>>> - drm_bridge_attach/detach. Those are meant to be called by the overall
> >>>>   drm driver to connect/disconnect a drm_bridge.
> >>>>
> >>>> - drm_bridge_add/remove. These are supposed to be called by the bridge
> >>>>   driver itself to register/unregister itself. Maybe we should rename
> >>>>   them, since the same issue happens with drm_panel, with the same
> >>>>   confusion.
> >>>>
> >>>> I thought someone was working on a cleanup series to fix this mess, but I
> >>>> didn't find anything.
> >>>
> >>> Ok, I just spotted the imbalance and didn't really dig into what
> >>> actually happens in these error paths. Now that I have done so I
> >>> believe that the removed drm_bridge_remove calls causes NULL
> >>> dereferences if/when the error paths are triggered.
> >>>
> >>> So, I don't think this can wait for some bigger cleanup.
> >>>
> >>> drm_bridge_remove calls list_del_init calls __list_del_entry calls
> >>> __list_del with NULL in both prev and next since the list member
> >>> is never initialized. prev and next are dereferenced by __list_del
> >>> and you have *boom*
> >>>
> >>> I recommend adding the tag
> >>>
> >>> Fixes: 84601dbdea36 ("drm: sti: rework init sequence")
> >>>
> >>> so that stable picks this one up.
> >>
> >> I just wanted to correct your commit message text - the correct solution
> >> is definitely _not_ for sti here to call drm_bridge_add.
> > 
> > Ah, I see what you mean. Do you want me to respin?
> 
> Hold on, no I don't agree. sti_hda.c does create a bridge for it's own
> internal use. It does not drm_bridge_add it, because all that ever does
> is adding the bridge to the global lost of bridges. But since this is
> a bridge for internal use, there is little point in calling drm_bridge_add,
> the driver currently gains nothing by doing so.
> 
> But, drm_bridge_add might be a good place to put common stuff for every
> bridge in the system, so it might be worthwhile to start requiring all
> bridges to be drm_bridge_add-ed. And IMHO, it would not be wrong to have
> the sti-hda driver call drm_bridge_add on the bridge it creates.
> 
> Do you really think it is actively wrong to call drm_bridge_add for
> internal bridges such as this?

If we want to share bridge init code, then I think we need a
drm_bridge_init(). Not overload drm_bridge_add (which really should be
drm_bridge_register I think, but oh well, it's at least consistent with
drm_panel_add).
-Daniel

> 
> Cheers,
> Peter
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Heiko Stuebner]

Am Mittwoch, 2. Mai 2018, 09:40:24 CEST schrieb Peter Rosin:
> drm_bridge_attach takes care of these assignments, so there is no need
> to open-code them a second time.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

applied to drm-misc-next


Thanks
Heiko

Re: [PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Heiko Stuebner]

Am Mittwoch, 2. Mai 2018, 09:40:24 CEST schrieb Peter Rosin:
> drm_bridge_attach takes care of these assignments, so there is no need
> to open-code them a second time.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

applied to drm-misc-next


Thanks
Heiko


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/rockchip: lvds: avoid duplicating drm_bridge_attach

[Heiko Stuebner]

Am Mittwoch, 2. Mai 2018, 09:40:24 CEST schrieb Peter Rosin:
> drm_bridge_attach takes care of these assignments, so there is no need
> to open-code them a second time.
> 
> Signed-off-by: Peter Rosin <peda@axentia.se>

applied to drm-misc-next


Thanks
Heiko