[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Douglas Anderson]

The drm_encoder_cleanup() was missing both from the error path of
dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
enabled and we ended up deferring probe of HDMI at boot.

This call isn't needed from unbind() because if dw_hdmi_bind() returns
no error then it takes over the job of freeing the encoder (in
dw_hdmi_unbind).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index 3d3cf2f8891e..88776aba984e 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -293,7 +293,16 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
 	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
 
-	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
+	ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
+
+	/*
+	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
+	 * which would have called the encoder cleanup.  Do it manually.
+	 */
+	if (ret)
+		drm_encoder_cleanup(encoder);
+
+	return ret;
 }
 
 static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
-- 
2.7.0.rc3.207.g0ac5344

[PATCH 2/2] drm/rockchip: vop: Fix vop crtc cleanup

[Douglas Anderson]

This fixes a few problems in the vop crtc cleanup (handling error
paths and cleanup upon exit):

* The vop_create_crtc() error path had an unsafe version of the
  iterator used for iterating over all planes (though it was
  destroying planes in the iterator so should have used the safe
  version)

* vop_destroy_crtc() - wasn't calling vop_plane_destroy(), which made
  slub_debug unhappy, at least if we ended up running this due to a
  deferred probe.

* In vop_create_crtc() if we were missing the "port" device tree node
  we would fail but not return an error (found by code inspection).

Fix these problems.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index fd370548d7d7..f86f797f10fd 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1108,7 +1108,7 @@ static int vop_create_crtc(struct vop *vop)
 	const struct vop_data *vop_data = vop->data;
 	struct device *dev = vop->dev;
 	struct drm_device *drm_dev = vop->drm_dev;
-	struct drm_plane *primary = NULL, *cursor = NULL, *plane;
+	struct drm_plane *primary = NULL, *cursor = NULL, *plane, *tmp;
 	struct drm_crtc *crtc = &vop->crtc;
 	struct device_node *port;
 	int ret;
@@ -1148,7 +1148,7 @@ static int vop_create_crtc(struct vop *vop)
 	ret = drm_crtc_init_with_planes(drm_dev, crtc, primary, cursor,
 					&vop_crtc_funcs, NULL);
 	if (ret)
-		return ret;
+		goto err_cleanup_planes;
 
 	drm_crtc_helper_add(crtc, &vop_crtc_helper_funcs);
 
@@ -1181,6 +1181,7 @@ static int vop_create_crtc(struct vop *vop)
 	if (!port) {
 		DRM_ERROR("no port node found in %s\n",
 			  dev->of_node->full_name);
+		ret = -ENOENT;
 		goto err_cleanup_crtc;
 	}
 
@@ -1194,7 +1195,8 @@ static int vop_create_crtc(struct vop *vop)
 err_cleanup_crtc:
 	drm_crtc_cleanup(crtc);
 err_cleanup_planes:
-	list_for_each_entry(plane, &drm_dev->mode_config.plane_list, head)
+	list_for_each_entry_safe(plane, tmp, &drm_dev->mode_config.plane_list,
+				 head)
 		drm_plane_cleanup(plane);
 	return ret;
 }
@@ -1202,9 +1204,28 @@ err_cleanup_planes:
 static void vop_destroy_crtc(struct vop *vop)
 {
 	struct drm_crtc *crtc = &vop->crtc;
+	struct drm_device *drm_dev = vop->drm_dev;
+	struct drm_plane *plane, *tmp;
 
 	rockchip_unregister_crtc_funcs(crtc);
 	of_node_put(crtc->port);
+
+	/*
+	 * We need to cleanup the planes now.  Why?
+	 *
+	 * The planes are "&vop->win[i].base".  That means the memory is
+	 * all part of the big "struct vop" chunk of memory.  That memory
+	 * was devm allocated and associated with this component.  We need to
+	 * free it ourselves before vop_unbind() finishes.
+	 */
+	list_for_each_entry_safe(plane, tmp, &drm_dev->mode_config.plane_list,
+				 head)
+		vop_plane_destroy(plane);
+
+	/*
+	 * Destroy CRTC after vop_plane_destroy() since vop_disable_plane()
+	 * references the CRTC.
+	 */
 	drm_crtc_cleanup(crtc);
 }
 
-- 
2.7.0.rc3.207.g0ac5344

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[John Keeping]

On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
> The drm_encoder_cleanup() was missing both from the error path of
> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
> enabled and we ended up deferring probe of HDMI at boot.
> 
> This call isn't needed from unbind() because if dw_hdmi_bind() returns
> no error then it takes over the job of freeing the encoder (in
> dw_hdmi_unbind).
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---

Does dw_hdmi-imx need a similar change?  I wonder if it would be cleaner
to push this into dw_hdmi_bind() if it affects all of the platforms..

>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 3d3cf2f8891e..88776aba984e 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -293,7 +293,16 @@ static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>  	drm_encoder_init(drm, encoder, &dw_hdmi_rockchip_encoder_funcs,
>  			 DRM_MODE_ENCODER_TMDS, NULL);
>  
> -	return dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
> +	ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq, plat_data);
> +
> +	/*
> +	 * If dw_hdmi_bind() fails we'll never call dw_hdmi_unbind(),
> +	 * which would have called the encoder cleanup.  Do it manually.
> +	 */
> +	if (ret)
> +		drm_encoder_cleanup(encoder);
> +
> +	return ret;
>  }
>  
>  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device *master,
> -- 
> 2.7.0.rc3.207.g0ac5344

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Russell King - ARM Linux]

On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
> > The drm_encoder_cleanup() was missing both from the error path of
> > dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
> > enabled and we ended up deferring probe of HDMI at boot.
> > 
> > This call isn't needed from unbind() because if dw_hdmi_bind() returns
> > no error then it takes over the job of freeing the encoder (in
> > dw_hdmi_unbind).
> > 
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> 
> Does dw_hdmi-imx need a similar change?  I wonder if it would be cleaner
> to push this into dw_hdmi_bind() if it affects all of the platforms..

I don't think moving it there would make sense - keep the initialisation
and cleanup together in the same file so that it's contained together.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Mark yao]

On 2016?03?05? 20:39, Russell King - ARM Linux wrote:
> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
>>> The drm_encoder_cleanup() was missing both from the error path of
>>> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
>>> enabled and we ended up deferring probe of HDMI at boot.
>>>
>>> This call isn't needed from unbind() because if dw_hdmi_bind() returns
>>> no error then it takes over the job of freeing the encoder (in
>>> dw_hdmi_unbind).
>>>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>> Does dw_hdmi-imx need a similar change?  I wonder if it would be cleaner
>> to push this into dw_hdmi_bind() if it affects all of the platforms..
> I don't think moving it there would make sense - keep the initialisation
> and cleanup together in the same file so that it's contained together.
>

I don't like this patch too, initialisation and cleanup not in the same 
file looks bad,

How about:

drivers/gpu/drm/bridge/dw-hdmi.c
void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
         hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);

hdmi->connector.funcs->destroy(&hdmi->connector);
-       hdmi->encoder->funcs->destroy(hdmi->encoder);

drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,

-       return dw_hdmi_bind(dev, master, data, encoder, iores, irq, 
plat_data);
+       ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq, 
plat_data);
+       if (ret)
+               drm_encoder_cleanup(encoder);
+
+       return ret;
  }

  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device 
*master,
                                     void *data)
  {
+       drm_encoder_cleanup(...);
         return dw_hdmi_unbind(dev, master, data);
  }

Thanks.

-- 
?ark Yao

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Doug Anderson]

Hi,

On Mon, Mar 7, 2016 at 12:37 AM, Mark yao <mark.yao@rock-chips.com> wrote:
> On 2016?03?05? 20:39, Russell King - ARM Linux wrote:
>>
>> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
>>>
>>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
>>>>
>>>> The drm_encoder_cleanup() was missing both from the error path of
>>>> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
>>>> enabled and we ended up deferring probe of HDMI at boot.
>>>>
>>>> This call isn't needed from unbind() because if dw_hdmi_bind() returns
>>>> no error then it takes over the job of freeing the encoder (in
>>>> dw_hdmi_unbind).
>>>>
>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>> ---
>>>
>>> Does dw_hdmi-imx need a similar change?  I wonder if it would be cleaner
>>> to push this into dw_hdmi_bind() if it affects all of the platforms..
>>
>> I don't think moving it there would make sense - keep the initialisation
>> and cleanup together in the same file so that it's contained together.
>>
>
> I don't like this patch too, initialisation and cleanup not in the same file
> looks bad,
>
> How about:
>
> drivers/gpu/drm/bridge/dw-hdmi.c
> void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
>         hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
>
> hdmi->connector.funcs->destroy(&hdmi->connector);
> -       hdmi->encoder->funcs->destroy(hdmi->encoder);
>
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> static int dw_hdmi_rockchip_bind(struct device *dev, struct device *master,
>
> -       return dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> plat_data);
> +       ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> plat_data);
> +       if (ret)
> +               drm_encoder_cleanup(encoder);
> +
> +       return ret;
>  }
>
>  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
> *master,
>                                     void *data)
>  {
> +       drm_encoder_cleanup(...);
>         return dw_hdmi_unbind(dev, master, data);
>  }

That'a a reasonable suggestion in theory.  ...but we run into the same
problem I've run into before with the strange relationship between
dw_hdmi and its descendants.

Specifically:

* "struct dw_hdmi", which has a pointer to encoder, is private to dw-hdmi.c

* We could get the encoder if we had a pointer to the "struct
rockchip_hdmi", but there's no way to get that.  You would _think_ you
could get it back using platform_get_drvdata() because it was stashed
with platform_set_drvdata().  ...but you'd be wrong.  The
platform_set_drvdata() is just there to fool you.  I believe when you
call dw_hdmi_bind() it clobbers your drvdata when it calls
dev_set_drvdata(dev, hdmi);


Said another way: taking your suggestion means we need to add some way
for dw_hdmi-rockchip.c to get a pointer to the encoder from a "struct
device".  We could (A) move the "struct dw_hdmi" definition to a
private header and allow dw_hdmi-rockchip.c to include it or we could
(B) add a dw_hdmi_get_encoder() API call that dw_hdmi-rockchip.c could
call.


If someone would let me know whether (A) or (B) is OK I'm happy to post a patch.


...or, of course, if I've made a mistake in all the above, feel free
to point it out.


-Doug

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Heiko Stübner]

Am Montag, 7. M?rz 2016, 09:36:07 schrieb Doug Anderson:
> Hi,
> 
> On Mon, Mar 7, 2016 at 12:37 AM, Mark yao <mark.yao@rock-chips.com> wrote:
> > On 2016?03?05? 20:39, Russell King - ARM Linux wrote:
> >> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
> >>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
> >>>> The drm_encoder_cleanup() was missing both from the error path of
> >>>> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
> >>>> enabled and we ended up deferring probe of HDMI at boot.
> >>>> 
> >>>> This call isn't needed from unbind() because if dw_hdmi_bind() returns
> >>>> no error then it takes over the job of freeing the encoder (in
> >>>> dw_hdmi_unbind).
> >>>> 
> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>>> ---
> >>> 
> >>> Does dw_hdmi-imx need a similar change?  I wonder if it would be cleaner
> >>> to push this into dw_hdmi_bind() if it affects all of the platforms..
> >> 
> >> I don't think moving it there would make sense - keep the initialisation
> >> and cleanup together in the same file so that it's contained together.
> > 
> > I don't like this patch too, initialisation and cleanup not in the same
> > file looks bad,
> > 
> > How about:
> > 
> > drivers/gpu/drm/bridge/dw-hdmi.c
> > void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
> > 
> >         hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
> > 
> > hdmi->connector.funcs->destroy(&hdmi->connector);
> > -       hdmi->encoder->funcs->destroy(hdmi->encoder);
> > 
> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > static int dw_hdmi_rockchip_bind(struct device *dev, struct device
> > *master,
> > 
> > -       return dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> > plat_data);
> > +       ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> > plat_data);
> > +       if (ret)
> > +               drm_encoder_cleanup(encoder);
> > +
> > +       return ret;
> > 
> >  }
> >  
> >  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
> > 
> > *master,
> > 
> >                                     void *data)
> >  
> >  {
> > 
> > +       drm_encoder_cleanup(...);
> > 
> >         return dw_hdmi_unbind(dev, master, data);
> >  
> >  }
> 
> That'a a reasonable suggestion in theory.  ...but we run into the same
> problem I've run into before with the strange relationship between
> dw_hdmi and its descendants.

I don't think handing off the cleanup responsibility is really in question 
here. I.e. I do believe it should also be fine to expect (as definition) the 
core driver to cleanup the encoder _after_ it sucessfully claimed it in 
dw_hdmi_bind().

We do the same in the rockchip power-domains, handing off the struct clk-
pointer to the pm_clk stuff (due to the clk-pointer being unique per-device 
nowadays).

So just making sure it is sucessfully handed off should also be ok.


Heiko

> 
> Specifically:
> 
> * "struct dw_hdmi", which has a pointer to encoder, is private to dw-hdmi.c
> 
> * We could get the encoder if we had a pointer to the "struct
> rockchip_hdmi", but there's no way to get that.  You would _think_ you
> could get it back using platform_get_drvdata() because it was stashed
> with platform_set_drvdata().  ...but you'd be wrong.  The
> platform_set_drvdata() is just there to fool you.  I believe when you
> call dw_hdmi_bind() it clobbers your drvdata when it calls
> dev_set_drvdata(dev, hdmi);
> 
> 
> Said another way: taking your suggestion means we need to add some way
> for dw_hdmi-rockchip.c to get a pointer to the encoder from a "struct
> device".  We could (A) move the "struct dw_hdmi" definition to a
> private header and allow dw_hdmi-rockchip.c to include it or we could
> (B) add a dw_hdmi_get_encoder() API call that dw_hdmi-rockchip.c could
> call.
> 
> 
> If someone would let me know whether (A) or (B) is OK I'm happy to post a
> patch.
> 
> 
> ...or, of course, if I've made a mistake in all the above, feel free
> to point it out.
> 
> 
> -Doug

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Doug Anderson]

Hi,

On Mon, Mar 7, 2016 at 9:57 AM, Heiko St?bner <heiko@sntech.de> wrote:
> Am Montag, 7. M?rz 2016, 09:36:07 schrieb Doug Anderson:
>> Hi,
>>
>> On Mon, Mar 7, 2016 at 12:37 AM, Mark yao <mark.yao@rock-chips.com> wrote:
>> > On 2016?03?05? 20:39, Russell King - ARM Linux wrote:
>> >> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
>> >>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
>> >>>> The drm_encoder_cleanup() was missing both from the error path of
>> >>>> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
>> >>>> enabled and we ended up deferring probe of HDMI at boot.
>> >>>>
>> >>>> This call isn't needed from unbind() because if dw_hdmi_bind() returns
>> >>>> no error then it takes over the job of freeing the encoder (in
>> >>>> dw_hdmi_unbind).
>> >>>>
>> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> >>>> ---
>> >>>
>> >>> Does dw_hdmi-imx need a similar change?  I wonder if it would be cleaner
>> >>> to push this into dw_hdmi_bind() if it affects all of the platforms..
>> >>
>> >> I don't think moving it there would make sense - keep the initialisation
>> >> and cleanup together in the same file so that it's contained together.
>> >
>> > I don't like this patch too, initialisation and cleanup not in the same
>> > file looks bad,
>> >
>> > How about:
>> >
>> > drivers/gpu/drm/bridge/dw-hdmi.c
>> > void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
>> >
>> >         hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
>> >
>> > hdmi->connector.funcs->destroy(&hdmi->connector);
>> > -       hdmi->encoder->funcs->destroy(hdmi->encoder);
>> >
>> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> > static int dw_hdmi_rockchip_bind(struct device *dev, struct device
>> > *master,
>> >
>> > -       return dw_hdmi_bind(dev, master, data, encoder, iores, irq,
>> > plat_data);
>> > +       ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq,
>> > plat_data);
>> > +       if (ret)
>> > +               drm_encoder_cleanup(encoder);
>> > +
>> > +       return ret;
>> >
>> >  }
>> >
>> >  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
>> >
>> > *master,
>> >
>> >                                     void *data)
>> >
>> >  {
>> >
>> > +       drm_encoder_cleanup(...);
>> >
>> >         return dw_hdmi_unbind(dev, master, data);
>> >
>> >  }
>>
>> That'a a reasonable suggestion in theory.  ...but we run into the same
>> problem I've run into before with the strange relationship between
>> dw_hdmi and its descendants.
>
> I don't think handing off the cleanup responsibility is really in question
> here. I.e. I do believe it should also be fine to expect (as definition) the
> core driver to cleanup the encoder _after_ it sucessfully claimed it in
> dw_hdmi_bind().
>
> We do the same in the rockchip power-domains, handing off the struct clk-
> pointer to the pm_clk stuff (due to the clk-pointer being unique per-device
> nowadays).
>
> So just making sure it is sucessfully handed off should also be ok.

If I understand correctly, that means you'd be OK with the original
patch I posted?  In that case cleanup continues to happen in the main
dw-hdmi.c if dw_hdmi_bind() succeeds and my patch fixes the cleanup
when dw_hdmi_bind() fails (and thus cleanup responsibility was not
handed off).

Also: I noticed that Russell also didn't seem to say that my original
patch was bad.  I think he just said that he didn't like John
Keeping's suggestion.

Please correct any misunderstandings.  Thanks!

-Doug

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Heiko Stübner]

Hi Doug,

Am Montag, 7. M?rz 2016, 10:49:53 schrieb Doug Anderson:
> On Mon, Mar 7, 2016 at 9:57 AM, Heiko St?bner <heiko@sntech.de> wrote:
> > Am Montag, 7. M?rz 2016, 09:36:07 schrieb Doug Anderson:
> >> Hi,
> >> 
> >> On Mon, Mar 7, 2016 at 12:37 AM, Mark yao <mark.yao@rock-chips.com> 
wrote:
> >> > On 2016?03?05? 20:39, Russell King - ARM Linux wrote:
> >> >> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
> >> >>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
> >> >>>> The drm_encoder_cleanup() was missing both from the error path of
> >> >>>> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
> >> >>>> enabled and we ended up deferring probe of HDMI at boot.
> >> >>>> 
> >> >>>> This call isn't needed from unbind() because if dw_hdmi_bind()
> >> >>>> returns
> >> >>>> no error then it takes over the job of freeing the encoder (in
> >> >>>> dw_hdmi_unbind).
> >> >>>> 
> >> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> >>>> ---
> >> >>> 
> >> >>> Does dw_hdmi-imx need a similar change?  I wonder if it would be
> >> >>> cleaner
> >> >>> to push this into dw_hdmi_bind() if it affects all of the platforms..
> >> >> 
> >> >> I don't think moving it there would make sense - keep the
> >> >> initialisation
> >> >> and cleanup together in the same file so that it's contained together.
> >> > 
> >> > I don't like this patch too, initialisation and cleanup not in the same
> >> > file looks bad,
> >> > 
> >> > How about:
> >> > 
> >> > drivers/gpu/drm/bridge/dw-hdmi.c
> >> > void dw_hdmi_unbind(struct device *dev, struct device *master, void
> >> > *data)
> >> > 
> >> >         hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
> >> > 
> >> > hdmi->connector.funcs->destroy(&hdmi->connector);
> >> > -       hdmi->encoder->funcs->destroy(hdmi->encoder);
> >> > 
> >> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> >> > static int dw_hdmi_rockchip_bind(struct device *dev, struct device
> >> > *master,
> >> > 
> >> > -       return dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> >> > plat_data);
> >> > +       ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> >> > plat_data);
> >> > +       if (ret)
> >> > +               drm_encoder_cleanup(encoder);
> >> > +
> >> > +       return ret;
> >> > 
> >> >  }
> >> >  
> >> >  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
> >> > 
> >> > *master,
> >> > 
> >> >                                     void *data)
> >> >  
> >> >  {
> >> > 
> >> > +       drm_encoder_cleanup(...);
> >> > 
> >> >         return dw_hdmi_unbind(dev, master, data);
> >> >  
> >> >  }
> >> 
> >> That'a a reasonable suggestion in theory.  ...but we run into the same
> >> problem I've run into before with the strange relationship between
> >> dw_hdmi and its descendants.
> > 
> > I don't think handing off the cleanup responsibility is really in question
> > here. I.e. I do believe it should also be fine to expect (as definition)
> > the core driver to cleanup the encoder _after_ it sucessfully claimed it
> > in dw_hdmi_bind().
> > 
> > We do the same in the rockchip power-domains, handing off the struct clk-
> > pointer to the pm_clk stuff (due to the clk-pointer being unique
> > per-device
> > nowadays).
> > 
> > So just making sure it is sucessfully handed off should also be ok.
> 
> If I understand correctly, that means you'd be OK with the original
> patch I posted?  In that case cleanup continues to happen in the main
> dw-hdmi.c if dw_hdmi_bind() succeeds and my patch fixes the cleanup
> when dw_hdmi_bind() fails (and thus cleanup responsibility was not
> handed off).

correct. I don't see the need to duplicate the cleanup (+added infrastructure 
to actually get the encoder in unbind) in all instances, if we just define that 
the dw_hdmi core takes control of the encoder _after_ it sucessfully bound.

So only if dw_hdmi_bind() fails does the hw-specific instance need to clean up 
the encoder it created.


> Also: I noticed that Russell also didn't seem to say that my original
> patch was bad.  I think he just said that he didn't like John
> Keeping's suggestion.

that was my reading as well.


Heiko

[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path

[Russell King - ARM Linux]

On Mon, Mar 07, 2016 at 07:56:18PM +0100, Heiko St?bner wrote:
> Hi Doug,
> 
> Am Montag, 7. M?rz 2016, 10:49:53 schrieb Doug Anderson:
> > On Mon, Mar 7, 2016 at 9:57 AM, Heiko St?bner <heiko@sntech.de> wrote:
> > > Am Montag, 7. M?rz 2016, 09:36:07 schrieb Doug Anderson:
> > >> Hi,
> > >> 
> > >> On Mon, Mar 7, 2016 at 12:37 AM, Mark yao <mark.yao@rock-chips.com> 
> wrote:
> > >> > On 2016?03?05? 20:39, Russell King - ARM Linux wrote:
> > >> >> On Sat, Mar 05, 2016 at 12:11:16PM +0000, John Keeping wrote:
> > >> >>> On Fri, Mar 04, 2016 at 03:22:01PM -0800, Douglas Anderson wrote:
> > >> >>>> The drm_encoder_cleanup() was missing both from the error path of
> > >> >>>> dw_hdmi_rockchip_bind().  This caused a crash when slub_debug was
> > >> >>>> enabled and we ended up deferring probe of HDMI at boot.
> > >> >>>> 
> > >> >>>> This call isn't needed from unbind() because if dw_hdmi_bind()
> > >> >>>> returns
> > >> >>>> no error then it takes over the job of freeing the encoder (in
> > >> >>>> dw_hdmi_unbind).
> > >> >>>> 
> > >> >>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > >> >>>> ---
> > >> >>> 
> > >> >>> Does dw_hdmi-imx need a similar change?  I wonder if it would be
> > >> >>> cleaner
> > >> >>> to push this into dw_hdmi_bind() if it affects all of the platforms..
> > >> >> 
> > >> >> I don't think moving it there would make sense - keep the
> > >> >> initialisation
> > >> >> and cleanup together in the same file so that it's contained together.
> > >> > 
> > >> > I don't like this patch too, initialisation and cleanup not in the same
> > >> > file looks bad,
> > >> > 
> > >> > How about:
> > >> > 
> > >> > drivers/gpu/drm/bridge/dw-hdmi.c
> > >> > void dw_hdmi_unbind(struct device *dev, struct device *master, void
> > >> > *data)
> > >> > 
> > >> >         hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
> > >> > 
> > >> > hdmi->connector.funcs->destroy(&hdmi->connector);
> > >> > -       hdmi->encoder->funcs->destroy(hdmi->encoder);
> > >> > 
> > >> > drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> > >> > static int dw_hdmi_rockchip_bind(struct device *dev, struct device
> > >> > *master,
> > >> > 
> > >> > -       return dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> > >> > plat_data);
> > >> > +       ret = dw_hdmi_bind(dev, master, data, encoder, iores, irq,
> > >> > plat_data);
> > >> > +       if (ret)
> > >> > +               drm_encoder_cleanup(encoder);
> > >> > +
> > >> > +       return ret;
> > >> > 
> > >> >  }
> > >> >  
> > >> >  static void dw_hdmi_rockchip_unbind(struct device *dev, struct device
> > >> > 
> > >> > *master,
> > >> > 
> > >> >                                     void *data)
> > >> >  
> > >> >  {
> > >> > 
> > >> > +       drm_encoder_cleanup(...);
> > >> > 
> > >> >         return dw_hdmi_unbind(dev, master, data);
> > >> >  
> > >> >  }
> > >> 
> > >> That'a a reasonable suggestion in theory.  ...but we run into the same
> > >> problem I've run into before with the strange relationship between
> > >> dw_hdmi and its descendants.
> > > 
> > > I don't think handing off the cleanup responsibility is really in question
> > > here. I.e. I do believe it should also be fine to expect (as definition)
> > > the core driver to cleanup the encoder _after_ it sucessfully claimed it
> > > in dw_hdmi_bind().
> > > 
> > > We do the same in the rockchip power-domains, handing off the struct clk-
> > > pointer to the pm_clk stuff (due to the clk-pointer being unique
> > > per-device
> > > nowadays).
> > > 
> > > So just making sure it is sucessfully handed off should also be ok.
> > 
> > If I understand correctly, that means you'd be OK with the original
> > patch I posted?  In that case cleanup continues to happen in the main
> > dw-hdmi.c if dw_hdmi_bind() succeeds and my patch fixes the cleanup
> > when dw_hdmi_bind() fails (and thus cleanup responsibility was not
> > handed off).
> 
> correct. I don't see the need to duplicate the cleanup (+added infrastructure 
> to actually get the encoder in unbind) in all instances, if we just define that 
> the dw_hdmi core takes control of the encoder _after_ it sucessfully bound.
> 
> So only if dw_hdmi_bind() fails does the hw-specific instance need to clean up 
> the encoder it created.
> 
> 
> > Also: I noticed that Russell also didn't seem to say that my original
> > patch was bad.  I think he just said that he didn't like John
> > Keeping's suggestion.
> 
> that was my reading as well.

The core code does not create the encoder - it merely stores a pointer
to the encoder, and registers itself as a bridge against the encoder.
It does call drm_mode_connector_attach_encoder() which adds the
encoder ID to the connector's possible encoders.

The cleanup (in dw_hdmi_unbind()) really ought to be moved out, but
that adds problems because of the need for two lots of driver data
to then be stored.

However, the bind() path really needs to be doing the encoder cleanup
before returning when an unsuccessful dw_hdmi_bind() returns.  I
wouldn't be surprised if there's a use-after-free bug here right now
because of the lack of that - core DRM will try to clean up the
attached encoder after it's been kfree()d.

Please, don't forget that the core driver gets used by two DRM drivers,
and if one driver suffers from a problem, it's possible that the other
driver does too - so both should be fixed.

This is very relevant in this case, the same author created the same bug
in both when dw-hdmi was split in order to support rockchip - the
history shows that when it was a single driver for imx-hdmi, it did
not have this issue (as there was no failure path possible after the
drm_encoder_init() call.)

So, please fix both rockchip and imx DRM drivers for this same bug.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.