From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko =?ISO-8859-1?Q?St=FCbner?= Subject: Re: [PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path Date: Mon, 07 Mar 2016 18:57:16 +0100 Message-ID: <1693114.6CV9KAkHOO@diego> References: <1457133723-24869-1-git-send-email-dianders@chromium.org> <56DD3DAF.8090700@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Doug Anderson Cc: Russell King - ARM Linux , John Keeping , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" , "open list:ARM/Rockchip SoC..." , "linux-arm-kernel@lists.infradead.org" List-Id: linux-rockchip.vger.kernel.org QW0gTW9udGFnLCA3LiBNw6RyeiAyMDE2LCAwOTozNjowNyBzY2hyaWViIERvdWcgQW5kZXJzb246 Cj4gSGksCj4gCj4gT24gTW9uLCBNYXIgNywgMjAxNiBhdCAxMjozNyBBTSwgTWFyayB5YW8gPG1h cmsueWFvQHJvY2stY2hpcHMuY29tPiB3cm90ZToKPiA+IE9uIDIwMTblubQwM+aciDA15pelIDIw OjM5LCBSdXNzZWxsIEtpbmcgLSBBUk0gTGludXggd3JvdGU6Cj4gPj4gT24gU2F0LCBNYXIgMDUs IDIwMTYgYXQgMTI6MTE6MTZQTSArMDAwMCwgSm9obiBLZWVwaW5nIHdyb3RlOgo+ID4+PiBPbiBG cmksIE1hciAwNCwgMjAxNiBhdCAwMzoyMjowMVBNIC0wODAwLCBEb3VnbGFzIEFuZGVyc29uIHdy b3RlOgo+ID4+Pj4gVGhlIGRybV9lbmNvZGVyX2NsZWFudXAoKSB3YXMgbWlzc2luZyBib3RoIGZy b20gdGhlIGVycm9yIHBhdGggb2YKPiA+Pj4+IGR3X2hkbWlfcm9ja2NoaXBfYmluZCgpLiAgVGhp cyBjYXVzZWQgYSBjcmFzaCB3aGVuIHNsdWJfZGVidWcgd2FzCj4gPj4+PiBlbmFibGVkIGFuZCB3 ZSBlbmRlZCB1cCBkZWZlcnJpbmcgcHJvYmUgb2YgSERNSSBhdCBib290Lgo+ID4+Pj4gCj4gPj4+ PiBUaGlzIGNhbGwgaXNuJ3QgbmVlZGVkIGZyb20gdW5iaW5kKCkgYmVjYXVzZSBpZiBkd19oZG1p X2JpbmQoKSByZXR1cm5zCj4gPj4+PiBubyBlcnJvciB0aGVuIGl0IHRha2VzIG92ZXIgdGhlIGpv YiBvZiBmcmVlaW5nIHRoZSBlbmNvZGVyIChpbgo+ID4+Pj4gZHdfaGRtaV91bmJpbmQpLgo+ID4+ Pj4gCj4gPj4+PiBTaWduZWQtb2ZmLWJ5OiBEb3VnbGFzIEFuZGVyc29uIDxkaWFuZGVyc0BjaHJv bWl1bS5vcmc+Cj4gPj4+PiAtLS0KPiA+Pj4gCj4gPj4+IERvZXMgZHdfaGRtaS1pbXggbmVlZCBh IHNpbWlsYXIgY2hhbmdlPyAgSSB3b25kZXIgaWYgaXQgd291bGQgYmUgY2xlYW5lcgo+ID4+PiB0 byBwdXNoIHRoaXMgaW50byBkd19oZG1pX2JpbmQoKSBpZiBpdCBhZmZlY3RzIGFsbCBvZiB0aGUg cGxhdGZvcm1zLi4KPiA+PiAKPiA+PiBJIGRvbid0IHRoaW5rIG1vdmluZyBpdCB0aGVyZSB3b3Vs ZCBtYWtlIHNlbnNlIC0ga2VlcCB0aGUgaW5pdGlhbGlzYXRpb24KPiA+PiBhbmQgY2xlYW51cCB0 b2dldGhlciBpbiB0aGUgc2FtZSBmaWxlIHNvIHRoYXQgaXQncyBjb250YWluZWQgdG9nZXRoZXIu Cj4gPiAKPiA+IEkgZG9uJ3QgbGlrZSB0aGlzIHBhdGNoIHRvbywgaW5pdGlhbGlzYXRpb24gYW5k IGNsZWFudXAgbm90IGluIHRoZSBzYW1lCj4gPiBmaWxlIGxvb2tzIGJhZCwKPiA+IAo+ID4gSG93 IGFib3V0Ogo+ID4gCj4gPiBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2R3LWhkbWkuYwo+ID4gdm9p ZCBkd19oZG1pX3VuYmluZChzdHJ1Y3QgZGV2aWNlICpkZXYsIHN0cnVjdCBkZXZpY2UgKm1hc3Rl ciwgdm9pZCAqZGF0YSkKPiA+IAo+ID4gICAgICAgICBoZG1pX3dyaXRlYihoZG1pLCB+MCwgSERN SV9JSF9NVVRFX1BIWV9TVEFUMCk7Cj4gPiAKPiA+IGhkbWktPmNvbm5lY3Rvci5mdW5jcy0+ZGVz dHJveSgmaGRtaS0+Y29ubmVjdG9yKTsKPiA+IC0gICAgICAgaGRtaS0+ZW5jb2Rlci0+ZnVuY3Mt PmRlc3Ryb3koaGRtaS0+ZW5jb2Rlcik7Cj4gPiAKPiA+IGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hp cC9kd19oZG1pLXJvY2tjaGlwLmMKPiA+IHN0YXRpYyBpbnQgZHdfaGRtaV9yb2NrY2hpcF9iaW5k KHN0cnVjdCBkZXZpY2UgKmRldiwgc3RydWN0IGRldmljZQo+ID4gKm1hc3RlciwKPiA+IAo+ID4g LSAgICAgICByZXR1cm4gZHdfaGRtaV9iaW5kKGRldiwgbWFzdGVyLCBkYXRhLCBlbmNvZGVyLCBp b3JlcywgaXJxLAo+ID4gcGxhdF9kYXRhKTsKPiA+ICsgICAgICAgcmV0ID0gZHdfaGRtaV9iaW5k KGRldiwgbWFzdGVyLCBkYXRhLCBlbmNvZGVyLCBpb3JlcywgaXJxLAo+ID4gcGxhdF9kYXRhKTsK PiA+ICsgICAgICAgaWYgKHJldCkKPiA+ICsgICAgICAgICAgICAgICBkcm1fZW5jb2Rlcl9jbGVh bnVwKGVuY29kZXIpOwo+ID4gKwo+ID4gKyAgICAgICByZXR1cm4gcmV0Owo+ID4gCj4gPiAgfQo+ ID4gIAo+ID4gIHN0YXRpYyB2b2lkIGR3X2hkbWlfcm9ja2NoaXBfdW5iaW5kKHN0cnVjdCBkZXZp Y2UgKmRldiwgc3RydWN0IGRldmljZQo+ID4gCj4gPiAqbWFzdGVyLAo+ID4gCj4gPiAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICB2b2lkICpkYXRhKQo+ID4gIAo+ID4gIHsKPiA+ IAo+ID4gKyAgICAgICBkcm1fZW5jb2Rlcl9jbGVhbnVwKC4uLik7Cj4gPiAKPiA+ICAgICAgICAg cmV0dXJuIGR3X2hkbWlfdW5iaW5kKGRldiwgbWFzdGVyLCBkYXRhKTsKPiA+ICAKPiA+ICB9Cj4g Cj4gVGhhdCdhIGEgcmVhc29uYWJsZSBzdWdnZXN0aW9uIGluIHRoZW9yeS4gIC4uLmJ1dCB3ZSBy dW4gaW50byB0aGUgc2FtZQo+IHByb2JsZW0gSSd2ZSBydW4gaW50byBiZWZvcmUgd2l0aCB0aGUg c3RyYW5nZSByZWxhdGlvbnNoaXAgYmV0d2Vlbgo+IGR3X2hkbWkgYW5kIGl0cyBkZXNjZW5kYW50 cy4KCkkgZG9uJ3QgdGhpbmsgaGFuZGluZyBvZmYgdGhlIGNsZWFudXAgcmVzcG9uc2liaWxpdHkg aXMgcmVhbGx5IGluIHF1ZXN0aW9uIApoZXJlLiBJLmUuIEkgZG8gYmVsaWV2ZSBpdCBzaG91bGQg YWxzbyBiZSBmaW5lIHRvIGV4cGVjdCAoYXMgZGVmaW5pdGlvbikgdGhlIApjb3JlIGRyaXZlciB0 byBjbGVhbnVwIHRoZSBlbmNvZGVyIF9hZnRlcl8gaXQgc3VjZXNzZnVsbHkgY2xhaW1lZCBpdCBp biAKZHdfaGRtaV9iaW5kKCkuCgpXZSBkbyB0aGUgc2FtZSBpbiB0aGUgcm9ja2NoaXAgcG93ZXIt ZG9tYWlucywgaGFuZGluZyBvZmYgdGhlIHN0cnVjdCBjbGstCnBvaW50ZXIgdG8gdGhlIHBtX2Ns ayBzdHVmZiAoZHVlIHRvIHRoZSBjbGstcG9pbnRlciBiZWluZyB1bmlxdWUgcGVyLWRldmljZSAK bm93YWRheXMpLgoKU28ganVzdCBtYWtpbmcgc3VyZSBpdCBpcyBzdWNlc3NmdWxseSBoYW5kZWQg b2ZmIHNob3VsZCBhbHNvIGJlIG9rLgoKCkhlaWtvCgo+IAo+IFNwZWNpZmljYWxseToKPiAKPiAq ICJzdHJ1Y3QgZHdfaGRtaSIsIHdoaWNoIGhhcyBhIHBvaW50ZXIgdG8gZW5jb2RlciwgaXMgcHJp dmF0ZSB0byBkdy1oZG1pLmMKPiAKPiAqIFdlIGNvdWxkIGdldCB0aGUgZW5jb2RlciBpZiB3ZSBo YWQgYSBwb2ludGVyIHRvIHRoZSAic3RydWN0Cj4gcm9ja2NoaXBfaGRtaSIsIGJ1dCB0aGVyZSdz IG5vIHdheSB0byBnZXQgdGhhdC4gIFlvdSB3b3VsZCBfdGhpbmtfIHlvdQo+IGNvdWxkIGdldCBp dCBiYWNrIHVzaW5nIHBsYXRmb3JtX2dldF9kcnZkYXRhKCkgYmVjYXVzZSBpdCB3YXMgc3Rhc2hl ZAo+IHdpdGggcGxhdGZvcm1fc2V0X2RydmRhdGEoKS4gIC4uLmJ1dCB5b3UnZCBiZSB3cm9uZy4g IFRoZQo+IHBsYXRmb3JtX3NldF9kcnZkYXRhKCkgaXMganVzdCB0aGVyZSB0byBmb29sIHlvdS4g IEkgYmVsaWV2ZSB3aGVuIHlvdQo+IGNhbGwgZHdfaGRtaV9iaW5kKCkgaXQgY2xvYmJlcnMgeW91 ciBkcnZkYXRhIHdoZW4gaXQgY2FsbHMKPiBkZXZfc2V0X2RydmRhdGEoZGV2LCBoZG1pKTsKPiAK PiAKPiBTYWlkIGFub3RoZXIgd2F5OiB0YWtpbmcgeW91ciBzdWdnZXN0aW9uIG1lYW5zIHdlIG5l ZWQgdG8gYWRkIHNvbWUgd2F5Cj4gZm9yIGR3X2hkbWktcm9ja2NoaXAuYyB0byBnZXQgYSBwb2lu dGVyIHRvIHRoZSBlbmNvZGVyIGZyb20gYSAic3RydWN0Cj4gZGV2aWNlIi4gIFdlIGNvdWxkIChB KSBtb3ZlIHRoZSAic3RydWN0IGR3X2hkbWkiIGRlZmluaXRpb24gdG8gYQo+IHByaXZhdGUgaGVh ZGVyIGFuZCBhbGxvdyBkd19oZG1pLXJvY2tjaGlwLmMgdG8gaW5jbHVkZSBpdCBvciB3ZSBjb3Vs ZAo+IChCKSBhZGQgYSBkd19oZG1pX2dldF9lbmNvZGVyKCkgQVBJIGNhbGwgdGhhdCBkd19oZG1p LXJvY2tjaGlwLmMgY291bGQKPiBjYWxsLgo+IAo+IAo+IElmIHNvbWVvbmUgd291bGQgbGV0IG1l IGtub3cgd2hldGhlciAoQSkgb3IgKEIpIGlzIE9LIEknbSBoYXBweSB0byBwb3N0IGEKPiBwYXRj aC4KPiAKPiAKPiAuLi5vciwgb2YgY291cnNlLCBpZiBJJ3ZlIG1hZGUgYSBtaXN0YWtlIGluIGFs bCB0aGUgYWJvdmUsIGZlZWwgZnJlZQo+IHRvIHBvaW50IGl0IG91dC4KPiAKPiAKPiAtRG91ZwoK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: heiko@sntech.de (Heiko =?ISO-8859-1?Q?St=FCbner?=) Date: Mon, 07 Mar 2016 18:57:16 +0100 Subject: [PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path In-Reply-To: References: <1457133723-24869-1-git-send-email-dianders@chromium.org> <56DD3DAF.8090700@rock-chips.com> Message-ID: <1693114.6CV9KAkHOO@diego> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752913AbcCGR5i (ORCPT ); Mon, 7 Mar 2016 12:57:38 -0500 Received: from gloria.sntech.de ([95.129.55.99]:57003 "EHLO gloria.sntech.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752538AbcCGR5b convert rfc822-to-8bit (ORCPT ); Mon, 7 Mar 2016 12:57:31 -0500 From: Heiko =?ISO-8859-1?Q?St=FCbner?= To: Doug Anderson 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" Subject: Re: [PATCH 1/2] drm/rockchip: dw_hdmi: Call drm_encoder_cleanup() in error path Date: Mon, 07 Mar 2016 18:57:16 +0100 Message-ID: <1693114.6CV9KAkHOO@diego> User-Agent: KMail/4.14.10 (Linux/4.2.0-1-amd64; KDE/4.14.14; x86_64; ; ) In-Reply-To: References: <1457133723-24869-1-git-send-email-dianders@chromium.org> <56DD3DAF.8090700@rock-chips.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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