[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path
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 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
wrote:
> >> > On 2016å¹´03æ05æ¥ 20:39, Russell King - ARM Linux wrote:
> >> >> On Sat, Mar 05, 2016 at 12:11:16PM +, 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
> >> ---
> >> >>>
> >> >>> 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
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 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
> wrote:
> > >> > On 2016å¹´03æ05æ¥ 20:39, Russell King - ARM Linux wrote:
> > >> >> On Sat, Mar 05, 2016 at 12:11:16PM +, 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
> > >> ---
> > >> >>>
> > >> >>> 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 u
[PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path
Am Montag, 7. März 2016, 09:36:07 schrieb Doug Anderson:
> Hi,
>
> On Mon, Mar 7, 2016 at 12:37 AM, Mark yao wrote:
> > On 2016å¹´03æ05æ¥ 20:39, Russell King - ARM Linux wrote:
> >> On Sat, Mar 05, 2016 at 12:11:16PM +, 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
> ---
> >>>
> >>> 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
On 2016å¹´03æ05æ¥ 20:39, Russell King - ARM Linux wrote:
> On Sat, Mar 05, 2016 at 12:11:16PM +, 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
>>> ---
>> 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
Hi,
On Mon, Mar 7, 2016 at 9:57 AM, Heiko Stübner 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 wrote:
>> > On 2016å¹´03æ05æ¥ 20:39, Russell King - ARM Linux wrote:
>> >> On Sat, Mar 05, 2016 at 12:11:16PM +, 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
>> ---
>> >>>
>> >>> 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
Hi,
On Mon, Mar 7, 2016 at 12:37 AM, Mark yao wrote:
> On 2016å¹´03æ05æ¥ 20:39, Russell King - ARM Linux wrote:
>>
>> On Sat, Mar 05, 2016 at 12:11:16PM +, 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
---
>>>
>>> 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
On Sat, Mar 05, 2016 at 12:11:16PM +, 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 > > --- > > 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
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 > --- 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
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 --- 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
