Revert "drm/imx: don't destroy mode objects manually on driver unbind"

Submitted by Stefan Agner on Oct. 16, 2018, 4:09 p.m.

Details

Message ID 20181016160923.2042-1-stefan@agner.ch
State New
Headers show
Series "Revert "drm/imx: don't destroy mode objects manually on driver unbind"" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Stefan Agner Oct. 16, 2018, 4:09 p.m.
This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.

Using the component framework requires all components to undo in
->unbind what ->bind does. Unfortunately that particular commit
broke this rule. In particular, this is an issue if a single
component during probe fails. In that case, component_bind_all()
calls unbind on already succussfully bound components, and then
frees memory allocated using devm. If one of those components
registered e.g. an encoder with the framework then this leads to
use after free situations.

Revert the commit to ensure that all components properly undo
what ->bind does.

Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
 drivers/gpu/drm/imx/imx-ldb.c          | 6 ++++++
 drivers/gpu/drm/imx/imx-tve.c          | 3 +++
 drivers/gpu/drm/imx/parallel-display.c | 3 +++
 4 files changed, 14 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 5ea0c82f9957..caa6061a98ba 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -305,11 +305,11 @@  static void imx_drm_unbind(struct device *dev)
 
 	drm_fb_cma_fbdev_fini(drm);
 
-	drm_mode_config_cleanup(drm);
-
 	component_unbind_all(drm->dev, drm);
 	dev_set_drvdata(dev, NULL);
 
+	drm_mode_config_cleanup(drm);
+
 	drm_dev_put(drm);
 }
 
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 3bd0f8a18e74..592aabc4a262 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -723,6 +723,12 @@  static void imx_ldb_unbind(struct device *dev, struct device *master,
 		if (channel->panel)
 			drm_panel_detach(channel->panel);
 
+		if (!channel->connector.funcs)
+			continue;
+
+		channel->connector.funcs->destroy(&channel->connector);
+		channel->encoder.funcs->destroy(&channel->encoder);
+
 		kfree(channel->edid);
 		i2c_put_adapter(channel->ddc);
 	}
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index cffd3310240e..8d6e89ce1edb 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -673,6 +673,9 @@  static void imx_tve_unbind(struct device *dev, struct device *master,
 {
 	struct imx_tve *tve = dev_get_drvdata(dev);
 
+	tve->connector.funcs->destroy(&tve->connector);
+	tve->encoder.funcs->destroy(&tve->encoder);
+
 	if (!IS_ERR(tve->dac_reg))
 		regulator_disable(tve->dac_reg);
 }
diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index aefd04e18f93..6f11bffcde37 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -258,6 +258,9 @@  static void imx_pd_unbind(struct device *dev, struct device *master,
 	if (imxpd->panel)
 		drm_panel_detach(imxpd->panel);
 
+	imxpd->encoder.funcs->destroy(&imxpd->encoder);
+	imxpd->connector.funcs->destroy(&imxpd->connector);
+
 	kfree(imxpd->edid);
 }
 

Comments

On Tue, Oct 16, 2018 at 06:09:23PM +0200, Stefan Agner wrote:
> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
> 
> Using the component framework requires all components to undo in
> ->unbind what ->bind does. Unfortunately that particular commit
> broke this rule. In particular, this is an issue if a single
> component during probe fails. In that case, component_bind_all()
> calls unbind on already succussfully bound components, and then
> frees memory allocated using devm. If one of those components
> registered e.g. an encoder with the framework then this leads to
> use after free situations.
> 
> Revert the commit to ensure that all components properly undo
> what ->bind does.
> 
> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Stefan Agner <stefan@agner.ch>

Adding Benjamin, who just made the same mistake I think (and I reviewed it
... oh well).
-Daniel

> ---
>  drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
>  drivers/gpu/drm/imx/imx-ldb.c          | 6 ++++++
>  drivers/gpu/drm/imx/imx-tve.c          | 3 +++
>  drivers/gpu/drm/imx/parallel-display.c | 3 +++
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 5ea0c82f9957..caa6061a98ba 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>  
>  	drm_fb_cma_fbdev_fini(drm);
>  
> -	drm_mode_config_cleanup(drm);
> -
>  	component_unbind_all(drm->dev, drm);
>  	dev_set_drvdata(dev, NULL);
>  
> +	drm_mode_config_cleanup(drm);
> +
>  	drm_dev_put(drm);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 3bd0f8a18e74..592aabc4a262 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
>  		if (channel->panel)
>  			drm_panel_detach(channel->panel);
>  
> +		if (!channel->connector.funcs)
> +			continue;
> +
> +		channel->connector.funcs->destroy(&channel->connector);
> +		channel->encoder.funcs->destroy(&channel->encoder);
> +
>  		kfree(channel->edid);
>  		i2c_put_adapter(channel->ddc);
>  	}
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index cffd3310240e..8d6e89ce1edb 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
>  {
>  	struct imx_tve *tve = dev_get_drvdata(dev);
>  
> +	tve->connector.funcs->destroy(&tve->connector);
> +	tve->encoder.funcs->destroy(&tve->encoder);
> +
>  	if (!IS_ERR(tve->dac_reg))
>  		regulator_disable(tve->dac_reg);
>  }
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index aefd04e18f93..6f11bffcde37 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
>  	if (imxpd->panel)
>  		drm_panel_detach(imxpd->panel);
>  
> +	imxpd->encoder.funcs->destroy(&imxpd->encoder);
> +	imxpd->connector.funcs->destroy(&imxpd->connector);
> +
>  	kfree(imxpd->edid);
>  }
>  
> -- 
> 2.19.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am Dienstag, den 16.10.2018, 18:09 +0200 schrieb Stefan Agner:
> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
> 
> Using the component framework requires all components to undo in
> ->unbind what ->bind does. Unfortunately that particular commit
> broke this rule. In particular, this is an issue if a single
> component during probe fails. In that case, component_bind_all()
> calls unbind on already succussfully bound components, and then
> frees memory allocated using devm. If one of those components
> registered e.g. an encoder with the framework then this leads to
> use after free situations.
> 
> Revert the commit to ensure that all components properly undo
> what ->bind does.

I agree with this patch in general, but I think I remember why I
changed this in the first place: dw_hdmi_imx_unbind does not destroy
the encoder that has been registered in dw_hdmi_imx_bind.

8e3b16e21174 was an (apparently wrong) attempt to fix such issues once
and for all. By reverting this commit we go back to leaking the HDMI
encoder, so I think this patch should include a fix to destroy the
encoder on unbind.

Regards,
Lucas

> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> > Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
>  drivers/gpu/drm/imx/imx-ldb.c          | 6 ++++++
>  drivers/gpu/drm/imx/imx-tve.c          | 3 +++
>  drivers/gpu/drm/imx/parallel-display.c | 3 +++
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 5ea0c82f9957..caa6061a98ba 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>  
> >  	drm_fb_cma_fbdev_fini(drm);
>  
> > -	drm_mode_config_cleanup(drm);
> -
> >  	component_unbind_all(drm->dev, drm);
> >  	dev_set_drvdata(dev, NULL);
>  
> > +	drm_mode_config_cleanup(drm);
> +
> >  	drm_dev_put(drm);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 3bd0f8a18e74..592aabc4a262 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
> >  		if (channel->panel)
> >  			drm_panel_detach(channel->panel);
>  
> > +		if (!channel->connector.funcs)
> > +			continue;
> +
> > +		channel->connector.funcs->destroy(&channel->connector);
> > +		channel->encoder.funcs->destroy(&channel->encoder);
> +
> >  		kfree(channel->edid);
> >  		i2c_put_adapter(channel->ddc);
> >  	}
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index cffd3310240e..8d6e89ce1edb 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
>  {
> >  	struct imx_tve *tve = dev_get_drvdata(dev);
>  
> > +	tve->connector.funcs->destroy(&tve->connector);
> > +	tve->encoder.funcs->destroy(&tve->encoder);
> +
> >  	if (!IS_ERR(tve->dac_reg))
> >  		regulator_disable(tve->dac_reg);
>  }
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index aefd04e18f93..6f11bffcde37 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
> >  	if (imxpd->panel)
> >  		drm_panel_detach(imxpd->panel);
>  
> > +	imxpd->encoder.funcs->destroy(&imxpd->encoder);
> > +	imxpd->connector.funcs->destroy(&imxpd->connector);
> +
> >  	kfree(imxpd->edid);
>  }
>
On 16.10.2018 18:51, Lucas Stach wrote:
> Am Dienstag, den 16.10.2018, 18:09 +0200 schrieb Stefan Agner:
>> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>>
>> Using the component framework requires all components to undo in
>> ->unbind what ->bind does. Unfortunately that particular commit
>> broke this rule. In particular, this is an issue if a single
>> component during probe fails. In that case, component_bind_all()
>> calls unbind on already succussfully bound components, and then
>> frees memory allocated using devm. If one of those components
>> registered e.g. an encoder with the framework then this leads to
>> use after free situations.
>>
>> Revert the commit to ensure that all components properly undo
>> what ->bind does.
> 
> I agree with this patch in general, but I think I remember why I
> changed this in the first place: dw_hdmi_imx_unbind does not destroy
> the encoder that has been registered in dw_hdmi_imx_bind.

Good point, yeah that is missing.

> 
> 8e3b16e21174 was an (apparently wrong) attempt to fix such issues once
> and for all. By reverting this commit we go back to leaking the HDMI
> encoder, so I think this patch should include a fix to destroy the
> encoder on unbind.

Makes sense, will add that to the same commit and send a v2.

--
Stefan

> 
> Regards,
> Lucas
> 
>> Link: https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
>> > Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
>> > Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
>>  drivers/gpu/drm/imx/imx-ldb.c          | 6 ++++++
>>  drivers/gpu/drm/imx/imx-tve.c          | 3 +++
>>  drivers/gpu/drm/imx/parallel-display.c | 3 +++
>>  4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 5ea0c82f9957..caa6061a98ba 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>>  
>> >  	drm_fb_cma_fbdev_fini(drm);
>>  
>> > -	drm_mode_config_cleanup(drm);
>> -
>> >  	component_unbind_all(drm->dev, drm);
>> >  	dev_set_drvdata(dev, NULL);
>>  
>> > +	drm_mode_config_cleanup(drm);
>> +
>> >  	drm_dev_put(drm);
>>  }
>>  
>> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
>> index 3bd0f8a18e74..592aabc4a262 100644
>> --- a/drivers/gpu/drm/imx/imx-ldb.c
>> +++ b/drivers/gpu/drm/imx/imx-ldb.c
>> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev, struct device *master,
>> >  		if (channel->panel)
>> >  			drm_panel_detach(channel->panel);
>>  
>> > +		if (!channel->connector.funcs)
>> > +			continue;
>> +
>> > +		channel->connector.funcs->destroy(&channel->connector);
>> > +		channel->encoder.funcs->destroy(&channel->encoder);
>> +
>> >  		kfree(channel->edid);
>> >  		i2c_put_adapter(channel->ddc);
>> >  	}
>> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
>> index cffd3310240e..8d6e89ce1edb 100644
>> --- a/drivers/gpu/drm/imx/imx-tve.c
>> +++ b/drivers/gpu/drm/imx/imx-tve.c
>> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev, struct device *master,
>>  {
>> >  	struct imx_tve *tve = dev_get_drvdata(dev);
>>  
>> > +	tve->connector.funcs->destroy(&tve->connector);
>> > +	tve->encoder.funcs->destroy(&tve->encoder);
>> +
>> >  	if (!IS_ERR(tve->dac_reg))
>> >  		regulator_disable(tve->dac_reg);
>>  }
>> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
>> index aefd04e18f93..6f11bffcde37 100644
>> --- a/drivers/gpu/drm/imx/parallel-display.c
>> +++ b/drivers/gpu/drm/imx/parallel-display.c
>> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev, struct device *master,
>> >  	if (imxpd->panel)
>> >  		drm_panel_detach(imxpd->panel);
>>  
>> > +	imxpd->encoder.funcs->destroy(&imxpd->encoder);
>> > +	imxpd->connector.funcs->destroy(&imxpd->connector);
>> +
>> >  	kfree(imxpd->edid);
>>  }
>>
On 16.10.2018 18:09, Stefan Agner wrote:
> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
> 
> Using the component framework requires all components to undo in
> ->unbind what ->bind does. Unfortunately that particular commit
> broke this rule. In particular, this is an issue if a single
> component during probe fails. In that case, component_bind_all()
> calls unbind on already succussfully bound components, and then
> frees memory allocated using devm. If one of those components
> registered e.g. an encoder with the framework then this leads to
> use after free situations.
> 
> Revert the commit to ensure that all components properly undo
> what ->bind does.

After Lucas comment mentioning HDMI unbind is not proper I looked
through all the unbind again. The other unbind functions need some
fixing too. I did not bother checking whether those were always broken
or just because things changed (the commit this is reverting was in
2016).... Here is what I found:

> 
> Link:
> https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
>  drivers/gpu/drm/imx/imx-ldb.c          | 6 ++++++
>  drivers/gpu/drm/imx/imx-tve.c          | 3 +++
>  drivers/gpu/drm/imx/parallel-display.c | 3 +++
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
> b/drivers/gpu/drm/imx/imx-drm-core.c
> index 5ea0c82f9957..caa6061a98ba 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>  
>  	drm_fb_cma_fbdev_fini(drm);
>  
> -	drm_mode_config_cleanup(drm);
> -
>  	component_unbind_all(drm->dev, drm);
>  	dev_set_drvdata(dev, NULL);
>  
> +	drm_mode_config_cleanup(drm);
> +
>  	drm_dev_put(drm);
>  }
>  
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 3bd0f8a18e74..592aabc4a262 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev,
> struct device *master,
>  		if (channel->panel)
>  			drm_panel_detach(channel->panel);
>  
> +		if (!channel->connector.funcs)
> +			continue;
> +
> +		channel->connector.funcs->destroy(&channel->connector);
> +		channel->encoder.funcs->destroy(&channel->encoder);

There can be an encoder and bridge, or an encoder and connector. All of
them should be properly cleaned up.

So I guess this should look like this:

  		if (channel->panel)
  			drm_panel_detach(channel->panel);

  		if (channel->bridge)
  			drm_bridge_detach(channel->bridge);

		if (channel->connector.funcs)
			channel->connector.funcs->destroy(&channel->connector);

		if (channel->encoder.funcs)
			channel->encoder.funcs->destroy(&channel->encoder);

		kfree(channel->edid);
		i2c_put_adapter(channel->ddc);

The last two functions following are only strictly necessary when
connector is initialized. But its safe to call them with null, so I
would just call them always.


> +
>  		kfree(channel->edid);
>  		i2c_put_adapter(channel->ddc);
>  	}
> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
> index cffd3310240e..8d6e89ce1edb 100644
> --- a/drivers/gpu/drm/imx/imx-tve.c
> +++ b/drivers/gpu/drm/imx/imx-tve.c
> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev,
> struct device *master,
>  {
>  	struct imx_tve *tve = dev_get_drvdata(dev);
>  
> +	tve->connector.funcs->destroy(&tve->connector);
> +	tve->encoder.funcs->destroy(&tve->encoder);
> +

Cleanup of tve->ddc missing.

>  	if (!IS_ERR(tve->dac_reg))
>  		regulator_disable(tve->dac_reg);
>  }
> diff --git a/drivers/gpu/drm/imx/parallel-display.c
> b/drivers/gpu/drm/imx/parallel-display.c
> index aefd04e18f93..6f11bffcde37 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev,
> struct device *master,
>  	if (imxpd->panel)
>  		drm_panel_detach(imxpd->panel);
>  

And in this case a bridge detach is missing.

Will send a v2 with that addressed.

--
Stefan

> +	imxpd->encoder.funcs->destroy(&imxpd->encoder);
> +	imxpd->connector.funcs->destroy(&imxpd->connector);
> +
>  	kfree(imxpd->edid);
>  }
On 17.10.2018 13:25, Stefan Agner wrote:
> On 16.10.2018 18:09, Stefan Agner wrote:
>> This reverts commit 8e3b16e2117409625b89807de3912ff773aea354.
>>
>> Using the component framework requires all components to undo in
>> ->unbind what ->bind does. Unfortunately that particular commit
>> broke this rule. In particular, this is an issue if a single
>> component during probe fails. In that case, component_bind_all()
>> calls unbind on already succussfully bound components, and then
>> frees memory allocated using devm. If one of those components
>> registered e.g. an encoder with the framework then this leads to
>> use after free situations.
>>
>> Revert the commit to ensure that all components properly undo
>> what ->bind does.
> 
> After Lucas comment mentioning HDMI unbind is not proper I looked
> through all the unbind again. The other unbind functions need some
> fixing too. I did not bother checking whether those were always broken
> or just because things changed (the commit this is reverting was in
> 2016).... Here is what I found:
> 
>>
>> Link:
>> https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg233327.html
>> Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/gpu/drm/imx/imx-drm-core.c     | 4 ++--
>>  drivers/gpu/drm/imx/imx-ldb.c          | 6 ++++++
>>  drivers/gpu/drm/imx/imx-tve.c          | 3 +++
>>  drivers/gpu/drm/imx/parallel-display.c | 3 +++
>>  4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c
>> b/drivers/gpu/drm/imx/imx-drm-core.c
>> index 5ea0c82f9957..caa6061a98ba 100644
>> --- a/drivers/gpu/drm/imx/imx-drm-core.c
>> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
>> @@ -305,11 +305,11 @@ static void imx_drm_unbind(struct device *dev)
>>
>>  	drm_fb_cma_fbdev_fini(drm);
>>
>> -	drm_mode_config_cleanup(drm);
>> -
>>  	component_unbind_all(drm->dev, drm);
>>  	dev_set_drvdata(dev, NULL);
>>
>> +	drm_mode_config_cleanup(drm);
>> +
>>  	drm_dev_put(drm);
>>  }
>>
>> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
>> index 3bd0f8a18e74..592aabc4a262 100644
>> --- a/drivers/gpu/drm/imx/imx-ldb.c
>> +++ b/drivers/gpu/drm/imx/imx-ldb.c
>> @@ -723,6 +723,12 @@ static void imx_ldb_unbind(struct device *dev,
>> struct device *master,
>>  		if (channel->panel)
>>  			drm_panel_detach(channel->panel);
>>
>> +		if (!channel->connector.funcs)
>> +			continue;
>> +
>> +		channel->connector.funcs->destroy(&channel->connector);
>> +		channel->encoder.funcs->destroy(&channel->encoder);
> 
> There can be an encoder and bridge, or an encoder and connector. All of
> them should be properly cleaned up.
> 
> So I guess this should look like this:
> 
>   		if (channel->panel)
>   			drm_panel_detach(channel->panel);
> 
>   		if (channel->bridge)
>   			drm_bridge_detach(channel->bridge);

Actually, when a bridge is attached to an encoder, which is the case
when unbind is getting called, then encoder cleanup takes care of
drm_bridge_detach.

> 
> 		if (channel->connector.funcs)
> 			channel->connector.funcs->destroy(&channel->connector);
> 
> 		if (channel->encoder.funcs)
> 			channel->encoder.funcs->destroy(&channel->encoder);
> 
> 		kfree(channel->edid);
> 		i2c_put_adapter(channel->ddc);
> 
> The last two functions following are only strictly necessary when
> connector is initialized. But its safe to call them with null, so I
> would just call them always.
> 
> 
>> +
>>  		kfree(channel->edid);
>>  		i2c_put_adapter(channel->ddc);
>>  	}
>> diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
>> index cffd3310240e..8d6e89ce1edb 100644
>> --- a/drivers/gpu/drm/imx/imx-tve.c
>> +++ b/drivers/gpu/drm/imx/imx-tve.c
>> @@ -673,6 +673,9 @@ static void imx_tve_unbind(struct device *dev,
>> struct device *master,
>>  {
>>  	struct imx_tve *tve = dev_get_drvdata(dev);
>>
>> +	tve->connector.funcs->destroy(&tve->connector);
>> +	tve->encoder.funcs->destroy(&tve->encoder);
>> +
> 
> Cleanup of tve->ddc missing.
> 
>>  	if (!IS_ERR(tve->dac_reg))
>>  		regulator_disable(tve->dac_reg);
>>  }
>> diff --git a/drivers/gpu/drm/imx/parallel-display.c
>> b/drivers/gpu/drm/imx/parallel-display.c
>> index aefd04e18f93..6f11bffcde37 100644
>> --- a/drivers/gpu/drm/imx/parallel-display.c
>> +++ b/drivers/gpu/drm/imx/parallel-display.c
>> @@ -258,6 +258,9 @@ static void imx_pd_unbind(struct device *dev,
>> struct device *master,
>>  	if (imxpd->panel)
>>  		drm_panel_detach(imxpd->panel);
>>
> 
> And in this case a bridge detach is missing.

Same here.

> 
> Will send a v2 with that addressed.

Still valid.

--
Stefan

> 
> --
> Stefan
> 
>> +	imxpd->encoder.funcs->destroy(&imxpd->encoder);
>> +	imxpd->connector.funcs->destroy(&imxpd->connector);
>> +
>>  	kfree(imxpd->edid);
>>  }