Received: by 2002:ac0:bc90:0:0:0:0:0 with SMTP id a16csp461959img; Fri, 22 Mar 2019 01:39:22 -0700 (PDT) X-Google-Smtp-Source: APXvYqwj/XFdo7icDQfEvMPEYI2OBuOCyxPGamugSuwWmb2mxl3/260V0JaBmLIAbddU5OPotYw8 X-Received: by 2002:a65:628f:: with SMTP id f15mr7862492pgv.410.1553243962408; Fri, 22 Mar 2019 01:39:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1553243962; cv=none; d=google.com; s=arc-20160816; b=P8/CqOgRaIckZXhNiVohMeyvQuYHYeA7JC4XANyTy4gCqGQMlq2OjVE89e7Rhs1enL 0z0vbY/jeoQoh8VvSh5lvbvw2KrZXK29pGy7HL6I1PRlMqnCB092PDBHm2qVPx2Cpmd9 WDLA5Xukf1ZQ/ioCgrd+vm01HMcO42//XmhxxjZMXnenEKRr4iUlhKwQ62zPCBTE86K9 kinD0+fJQp7rOfGU5MV1SzTdrVeTJbBR0NOaJAwOUtJQWCI8wvjvIjWtUd6JkX1corL6 Ao6VPdMAJMI/VB4Um2zE179ziouqz+loSmUFaVhGMJ/yHj67fraSlUOB4a1VYe1czZip EA3Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=PuEHL9v5y7xgSUkoNCvH+24IuMEC6czL1Cyv2E60c9c=; b=CRcn/YHdViC1WQ5W6aYvORABr9gg7laLFGC+5KTvOTSJJKlfTvqatgZAuSmAqaTwvd KMM7aha/WpVAQ5rzTNmY1mx6UMM/TPr0eV26jm3yWkP5A/BvXmO5LWeTsCRhxYmJr7fo 1ycwIpsKxOdPJcDW8upAhs6dHzrLOMoqsM8lkUnrEU2r3thGA42V0fDiIX0UjTexqMQ8 QhfG+q99hc7wWWwsOn2vpI/4XbCEueYxlMWAtQOS1J3tImGt4dbMuZdGUDY40tSuMZcy h99MTv4X74a4yUaKrpIWEnQ8598hEjOoEakf4We3V3/R7NKgkzU8KDzqnYAT9bTkV+DX +PSA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 97si6764446plb.407.2019.03.22.01.39.07; Fri, 22 Mar 2019 01:39:22 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727832AbfCVIiW (ORCPT + 99 others); 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