Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753521AbcCGSuB (ORCPT ); Mon, 7 Mar 2016 13:50:01 -0500 Received: from mail-vk0-f45.google.com ([209.85.213.45]:34468 "EHLO mail-vk0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753260AbcCGSty convert rfc822-to-8bit (ORCPT ); Mon, 7 Mar 2016 13:49:54 -0500 MIME-Version: 1.0 In-Reply-To: <1693114.6CV9KAkHOO@diego> References: <1457133723-24869-1-git-send-email-dianders@chromium.org> <56DD3DAF.8090700@rock-chips.com> <1693114.6CV9KAkHOO@diego> Date: Mon, 7 Mar 2016 10:49:53 -0800 X-Google-Sender-Auth: 0OUCJk250LOwvtZnO-ZHkTIKdLE Message-ID: Subject: Re: [PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path From: Doug Anderson To: =?UTF-8?Q?Heiko_St=C3=BCbner?= Cc: Mark yao , Russell King - ARM Linux , John Keeping , David Airlie , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "open list:ARM/Rockchip SoC..." , Daniel Kurtz , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3565 Lines: 97 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 +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 >> >>>> --- >> >>> >> >>> 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