From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81600C43381 for ; Fri, 22 Mar 2019 08:38:24 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 53EAB218D3 for ; Fri, 22 Mar 2019 08:38:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727845AbfCVIiW (ORCPT ); Fri, 22 Mar 2019 04:38:22 -0400 Received: from mga06.intel.com ([134.134.136.31]:44343 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727440AbfCVIiW (ORCPT ); Fri, 22 Mar 2019 04:38:22 -0400 X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Mar 2019 01:38:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.60,256,1549958400"; d="scan'208";a="157331610" Received: from kuha.fi.intel.com ([10.237.72.189]) by fmsmga001.fm.intel.com with SMTP; 22 Mar 2019 01:38:18 -0700 Received: by kuha.fi.intel.com (sSMTP sendmail emulation); Fri, 22 Mar 2019 10:38:17 +0200 Date: Fri, 22 Mar 2019 10:38:17 +0200 From: Heikki Krogerus To: Marc Zyngier Cc: Greg Kroah-Hartman , Guenter Roeck , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] usb: typec: fusb302: Fix debugfs mutex initialisation Message-ID: <20190322083817.GA17486@kuha.fi.intel.com> References: <20190318174906.10429-1-marc.zyngier@arm.com> <20190319114511.GS7752@kuha.fi.intel.com> <20190320160708.5f31ff10@why.wild-wind.fr.eu.org> <20190320163433.GD7752@kuha.fi.intel.com> <20190321132418.GE7752@kuha.fi.intel.com> <20190321160227.32d32bba@why.wild-wind.fr.eu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190321160227.32d32bba@why.wild-wind.fr.eu.org> User-Agent: Mutt/1.11.3 (2019-02-01) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Mar 21, 2019 at 04:02:27PM +0000, Marc Zyngier wrote: > On Thu, 21 Mar 2019 15:24:18 +0200 > Heikki Krogerus wrote: > > > Hi, > > > > On Wed, Mar 20, 2019 at 06:34:33PM +0200, Heikki Krogerus wrote: > > > > > After applying this there was no more "fusb302" debugfs directory, and > > > > > attempt to unload the fusb302 module dead locked. Also, attempt to > > > > > reboot caused this to happen on my GDPWin board after applying the > > > > > patch: > > > > > > > > > > BUG: Dentry 0000000012f2a05d{i=149,n=i2c-fusb302} still in use (1) [unmount of sysfs sysfs] > > > > > WARNING: CPU: 3 PID: 1639 at fs/dcache.c:1529 umount_check.cold.55+0x2e/0x3a > > > > > Modules linked in: intel_xhci_usb_role_switch roles pi3usb30532 typec i915 intel_gtt intel_cht_int33fe [last unloaded: tcpm] > > > > > CPU: 3 PID: 1639 Comm: umount Not tainted 5.1.0-rc1-heikki+ #916 > > > > > Hardware name: Default string Default string/Default string, BIOS 5.11 05/25/2017 > > > > > RIP: 0010:umount_check.cold.55+0x2e/0x3a > > > > > ... > > > > > > > > > > Note. Your patch has also a conflict with patches from Hans, I > > > > > think with this one: https://patchwork.kernel.org/patch/10847275/ > > > > > I can take care of that, but you can also rebase the next version on > > > > > top of my typec-next branch to solve that problem: > > > > > https://github.com/krohei/linux/commits/typec-next > > > > > > > > OK, this is very weird. I can't reproduce any of the issues you're > > > > reporting: > > > > > > > > - the patch applies cleanly on top of typec-next > > > > - removing the fusb302 module works > > > > - I see the debugfs file whenever fsusb302 is inserted > > > > > > > > Maybe you were trying this on another branch? > > > > > > No, the branch is correct. Actually, I tested this on top of mainline > > > and linux-next. I saw that happen on both. > > > > > > On these Intel Cherrytrail based boards like my GDBWin, fusb302 is one > > > of the functions of a weir MFD device (the driver for that device is > > > drivers/platform/x86/intel_cht_int33fe.c). It's entirely possible that > > > we are doing something wrong in that driver, and your patch just makes > > > the problem visible. > > > > > > I'll continue debugging. > > > > I figured out what's the problem. It seems that the driver does not > > probe successfully, which is why I don't see that "fusb302" debugfs > > directory. > > > > The reason is that if tcpm_register_port() returns with -EPROBE_DEFER, > > we end up with that rootdir already pointing to something, even though > > the entry is destroyed in that case. So next time the driver is > > probed, that "fusb302" directory does get created as rootdir has a > > value, and debugfs_create_file() fails. > > > > I think the correct fix is to just initialize the mutex earlier. > > Something like this should work: > > > > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c > > index 261b82900fec..8e43ea27f26d 100644 > > --- a/drivers/usb/typec/tcpm/fusb302.c > > +++ b/drivers/usb/typec/tcpm/fusb302.c > > @@ -211,7 +211,6 @@ static struct dentry *rootdir; > > > > static void fusb302_debugfs_init(struct fusb302_chip *chip) > > { > > - mutex_init(&chip->logbuffer_lock); > > if (!rootdir) > > rootdir = debugfs_create_dir("fusb302", NULL); > > > > @@ -1667,6 +1666,7 @@ static int fusb302_probe(struct i2c_client *client, > > chip->tcpc_config = fusb302_tcpc_config; > > chip->tcpc_dev.config = &chip->tcpc_config; > > mutex_init(&chip->lock); > > + mutex_init(&chip->logbuffer_lock); > > > > chip->tcpc_dev.fwnode = > > device_get_named_child_node(dev, "connector"); > > Looks good to me, although you probably want to make that conditional > on CONFIG_DEBUG_FS being set. Just move that logbuffer_lock member outside of the ifdef CONFIG_DEBUG_FS condition. For the record, I don't see any use for those ifdef checks. Those logbuffer members in struct fusb302_chip could be kept in their own structure, for example struct fusb302_log, that we allocate separately and only if debugfs_initialized() returns true. thanks, -- heikki From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2] usb: typec: fusb302: Fix debugfs mutex initialisation From: Heikki Krogerus Message-Id: <20190322083817.GA17486@kuha.fi.intel.com> Date: Fri, 22 Mar 2019 10:38:17 +0200 To: Marc Zyngier Cc: Greg Kroah-Hartman , Guenter Roeck , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gVGh1LCBNYXIgMjEsIDIwMTkgYXQgMDQ6MDI6MjdQTSArMDAwMCwgTWFyYyBaeW5naWVyIHdy b3RlOgo+IE9uIFRodSwgMjEgTWFyIDIwMTkgMTU6MjQ6MTggKzAyMDAKPiBIZWlra2kgS3JvZ2Vy dXMgPGhlaWtraS5rcm9nZXJ1c0BsaW51eC5pbnRlbC5jb20+IHdyb3RlOgo+IAo+ID4gSGksCj4g PiAKPiA+IE9uIFdlZCwgTWFyIDIwLCAyMDE5IGF0IDA2OjM0OjMzUE0gKzAyMDAsIEhlaWtraSBL cm9nZXJ1cyB3cm90ZToKPiA+ID4gPiA+IEFmdGVyIGFwcGx5aW5nIHRoaXMgdGhlcmUgd2FzIG5v IG1vcmUgImZ1c2IzMDIiIGRlYnVnZnMgZGlyZWN0b3J5LCBhbmQKPiA+ID4gPiA+IGF0dGVtcHQg dG8gdW5sb2FkIHRoZSBmdXNiMzAyIG1vZHVsZSBkZWFkIGxvY2tlZC4gQWxzbywgYXR0ZW1wdCB0 bwo+ID4gPiA+ID4gcmVib290IGNhdXNlZCB0aGlzIHRvIGhhcHBlbiBvbiBteSBHRFBXaW4gYm9h cmQgYWZ0ZXIgYXBwbHlpbmcgdGhlCj4gPiA+ID4gPiBwYXRjaDoKPiA+ID4gPiA+IAo+ID4gPiA+ ID4gICAgICAgICBCVUc6IERlbnRyeSAwMDAwMDAwMDEyZjJhMDVke2k9MTQ5LG49aTJjLWZ1c2Iz MDJ9ICBzdGlsbCBpbiB1c2UgKDEpIFt1bm1vdW50IG9mIHN5c2ZzIHN5c2ZzXQo+ID4gPiA+ID4g ICAgICAgICBXQVJOSU5HOiBDUFU6IDMgUElEOiAxNjM5IGF0IGZzL2RjYWNoZS5jOjE1MjkgdW1v dW50X2NoZWNrLmNvbGQuNTUrMHgyZS8weDNhCj4gPiA+ID4gPiAgICAgICAgIE1vZHVsZXMgbGlu a2VkIGluOiBpbnRlbF94aGNpX3VzYl9yb2xlX3N3aXRjaCByb2xlcyBwaTN1c2IzMDUzMiB0eXBl YyBpOTE1IGludGVsX2d0dCBpbnRlbF9jaHRfaW50MzNmZSBbbGFzdCB1bmxvYWRlZDogdGNwbV0K PiA+ID4gPiA+ICAgICAgICAgQ1BVOiAzIFBJRDogMTYzOSBDb21tOiB1bW91bnQgTm90IHRhaW50 ZWQgNS4xLjAtcmMxLWhlaWtraSsgIzkxNgo+ID4gPiA+ID4gICAgICAgICBIYXJkd2FyZSBuYW1l OiBEZWZhdWx0IHN0cmluZyBEZWZhdWx0IHN0cmluZy9EZWZhdWx0IHN0cmluZywgQklPUyA1LjEx IDA1LzI1LzIwMTcKPiA+ID4gPiA+ICAgICAgICAgUklQOiAwMDEwOnVtb3VudF9jaGVjay5jb2xk LjU1KzB4MmUvMHgzYQo+ID4gPiA+ID4gICAgICAgICAuLi4KPiA+ID4gPiA+IAo+ID4gPiA+ID4g Tm90ZS4gWW91ciBwYXRjaCBoYXMgYWxzbyBhIGNvbmZsaWN0IHdpdGggcGF0Y2hlcyBmcm9tIEhh bnMsIEkKPiA+ID4gPiA+IHRoaW5rIHdpdGggdGhpcyBvbmU6IGh0dHBzOi8vcGF0Y2h3b3JrLmtl cm5lbC5vcmcvcGF0Y2gvMTA4NDcyNzUvCj4gPiA+ID4gPiBJIGNhbiB0YWtlIGNhcmUgb2YgdGhh dCwgYnV0IHlvdSBjYW4gYWxzbyByZWJhc2UgdGhlIG5leHQgdmVyc2lvbiBvbgo+ID4gPiA+ID4g dG9wIG9mIG15IHR5cGVjLW5leHQgYnJhbmNoIHRvIHNvbHZlIHRoYXQgcHJvYmxlbToKPiA+ID4g PiA+IGh0dHBzOi8vZ2l0aHViLmNvbS9rcm9oZWkvbGludXgvY29tbWl0cy90eXBlYy1uZXh0ICAK PiA+ID4gPiAKPiA+ID4gPiBPSywgdGhpcyBpcyB2ZXJ5IHdlaXJkLiBJIGNhbid0IHJlcHJvZHVj ZSBhbnkgb2YgdGhlIGlzc3VlcyB5b3UncmUKPiA+ID4gPiByZXBvcnRpbmc6Cj4gPiA+ID4gCj4g PiA+ID4gLSB0aGUgcGF0Y2ggYXBwbGllcyBjbGVhbmx5IG9uIHRvcCBvZiB0eXBlYy1uZXh0Cj4g PiA+ID4gLSByZW1vdmluZyB0aGUgZnVzYjMwMiBtb2R1bGUgd29ya3MKPiA+ID4gPiAtIEkgc2Vl IHRoZSBkZWJ1Z2ZzIGZpbGUgd2hlbmV2ZXIgZnN1c2IzMDIgaXMgaW5zZXJ0ZWQKPiA+ID4gPiAK PiA+ID4gPiBNYXliZSB5b3Ugd2VyZSB0cnlpbmcgdGhpcyBvbiBhbm90aGVyIGJyYW5jaD8gIAo+ ID4gPiAKPiA+ID4gTm8sIHRoZSBicmFuY2ggaXMgY29ycmVjdC4gQWN0dWFsbHksIEkgdGVzdGVk IHRoaXMgb24gdG9wIG9mIG1haW5saW5lCj4gPiA+IGFuZCBsaW51eC1uZXh0LiBJIHNhdyB0aGF0 IGhhcHBlbiBvbiBib3RoLgo+ID4gPiAKPiA+ID4gT24gdGhlc2UgSW50ZWwgQ2hlcnJ5dHJhaWwg YmFzZWQgYm9hcmRzIGxpa2UgbXkgR0RCV2luLCBmdXNiMzAyIGlzIG9uZQo+ID4gPiBvZiB0aGUg ZnVuY3Rpb25zIG9mIGEgd2VpciBNRkQgZGV2aWNlICh0aGUgZHJpdmVyIGZvciB0aGF0IGRldmlj ZSBpcwo+ID4gPiBkcml2ZXJzL3BsYXRmb3JtL3g4Ni9pbnRlbF9jaHRfaW50MzNmZS5jKS4gSXQn cyBlbnRpcmVseSBwb3NzaWJsZSB0aGF0Cj4gPiA+IHdlIGFyZSBkb2luZyBzb21ldGhpbmcgd3Jv bmcgaW4gdGhhdCBkcml2ZXIsIGFuZCB5b3VyIHBhdGNoIGp1c3QgbWFrZXMKPiA+ID4gdGhlIHBy b2JsZW0gdmlzaWJsZS4KPiA+ID4gCj4gPiA+IEknbGwgY29udGludWUgZGVidWdnaW5nLiAgCj4g PiAKPiA+IEkgZmlndXJlZCBvdXQgd2hhdCdzIHRoZSBwcm9ibGVtLiBJdCBzZWVtcyB0aGF0IHRo ZSBkcml2ZXIgZG9lcyBub3QKPiA+IHByb2JlIHN1Y2Nlc3NmdWxseSwgd2hpY2ggaXMgd2h5IEkg ZG9uJ3Qgc2VlIHRoYXQgImZ1c2IzMDIiIGRlYnVnZnMKPiA+IGRpcmVjdG9yeS4KPiA+IAo+ID4g VGhlIHJlYXNvbiBpcyB0aGF0IGlmIHRjcG1fcmVnaXN0ZXJfcG9ydCgpIHJldHVybnMgd2l0aCAt RVBST0JFX0RFRkVSLAo+ID4gd2UgZW5kIHVwIHdpdGggdGhhdCByb290ZGlyIGFscmVhZHkgcG9p bnRpbmcgdG8gc29tZXRoaW5nLCBldmVuIHRob3VnaAo+ID4gdGhlIGVudHJ5IGlzIGRlc3Ryb3ll ZCBpbiB0aGF0IGNhc2UuIFNvIG5leHQgdGltZSB0aGUgZHJpdmVyIGlzCj4gPiBwcm9iZWQsIHRo YXQgImZ1c2IzMDIiIGRpcmVjdG9yeSBkb2VzIGdldCBjcmVhdGVkIGFzIHJvb3RkaXIgaGFzIGEK PiA+IHZhbHVlLCBhbmQgZGVidWdmc19jcmVhdGVfZmlsZSgpIGZhaWxzLgo+ID4gCj4gPiBJIHRo aW5rIHRoZSBjb3JyZWN0IGZpeCBpcyB0byBqdXN0IGluaXRpYWxpemUgdGhlIG11dGV4IGVhcmxp ZXIuCj4gPiBTb21ldGhpbmcgbGlrZSB0aGlzIHNob3VsZCB3b3JrOgo+ID4gCj4gPiBkaWZmIC0t Z2l0IGEvZHJpdmVycy91c2IvdHlwZWMvdGNwbS9mdXNiMzAyLmMgYi9kcml2ZXJzL3VzYi90eXBl Yy90Y3BtL2Z1c2IzMDIuYwo+ID4gaW5kZXggMjYxYjgyOTAwZmVjLi44ZTQzZWEyN2YyNmQgMTAw NjQ0Cj4gPiAtLS0gYS9kcml2ZXJzL3VzYi90eXBlYy90Y3BtL2Z1c2IzMDIuYwo+ID4gKysrIGIv ZHJpdmVycy91c2IvdHlwZWMvdGNwbS9mdXNiMzAyLmMKPiA+IEBAIC0yMTEsNyArMjExLDYgQEAg c3RhdGljIHN0cnVjdCBkZW50cnkgKnJvb3RkaXI7Cj4gPiAgCj4gPiAgc3RhdGljIHZvaWQgZnVz YjMwMl9kZWJ1Z2ZzX2luaXQoc3RydWN0IGZ1c2IzMDJfY2hpcCAqY2hpcCkKPiA+ICB7Cj4gPiAt ICAgICAgIG11dGV4X2luaXQoJmNoaXAtPmxvZ2J1ZmZlcl9sb2NrKTsKPiA+ICAgICAgICAgaWYg KCFyb290ZGlyKQo+ID4gICAgICAgICAgICAgICAgIHJvb3RkaXIgPSBkZWJ1Z2ZzX2NyZWF0ZV9k aXIoImZ1c2IzMDIiLCBOVUxMKTsKPiA+ICAKPiA+IEBAIC0xNjY3LDYgKzE2NjYsNyBAQCBzdGF0 aWMgaW50IGZ1c2IzMDJfcHJvYmUoc3RydWN0IGkyY19jbGllbnQgKmNsaWVudCwKPiA+ICAgICAg ICAgY2hpcC0+dGNwY19jb25maWcgPSBmdXNiMzAyX3RjcGNfY29uZmlnOwo+ID4gICAgICAgICBj aGlwLT50Y3BjX2Rldi5jb25maWcgPSAmY2hpcC0+dGNwY19jb25maWc7Cj4gPiAgICAgICAgIG11 dGV4X2luaXQoJmNoaXAtPmxvY2spOwo+ID4gKyAgICAgICBtdXRleF9pbml0KCZjaGlwLT5sb2di dWZmZXJfbG9jayk7Cj4gPiAgCj4gPiAgICAgICAgIGNoaXAtPnRjcGNfZGV2LmZ3bm9kZSA9Cj4g PiAgICAgICAgICAgICAgICAgZGV2aWNlX2dldF9uYW1lZF9jaGlsZF9ub2RlKGRldiwgImNvbm5l Y3RvciIpOwo+IAo+IExvb2tzIGdvb2QgdG8gbWUsIGFsdGhvdWdoIHlvdSBwcm9iYWJseSB3YW50 IHRvIG1ha2UgdGhhdCBjb25kaXRpb25hbAo+IG9uIENPTkZJR19ERUJVR19GUyBiZWluZyBzZXQu CgpKdXN0IG1vdmUgdGhhdCBsb2didWZmZXJfbG9jayBtZW1iZXIgb3V0c2lkZSBvZiB0aGUgaWZk ZWYKQ09ORklHX0RFQlVHX0ZTIGNvbmRpdGlvbi4KCkZvciB0aGUgcmVjb3JkLCBJIGRvbid0IHNl ZSBhbnkgdXNlIGZvciB0aG9zZSBpZmRlZiBjaGVja3MuIFRob3NlCmxvZ2J1ZmZlciBtZW1iZXJz IGluIHN0cnVjdCBmdXNiMzAyX2NoaXAgY291bGQgYmUga2VwdCBpbiB0aGVpciBvd24Kc3RydWN0 dXJlLCBmb3IgZXhhbXBsZSBzdHJ1Y3QgZnVzYjMwMl9sb2csIHRoYXQgd2UgYWxsb2NhdGUKc2Vw YXJhdGVseSBhbmQgb25seSBpZiBkZWJ1Z2ZzX2luaXRpYWxpemVkKCkgcmV0dXJucyB0cnVlLgoK CnRoYW5rcywK