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=-2.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,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 C8C47C6778A for ; Sat, 7 Jul 2018 14:26:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7F36A20870 for ; Sat, 7 Jul 2018 14:26:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7F36A20870 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753937AbeGGOZ7 (ORCPT ); Sat, 7 Jul 2018 10:25:59 -0400 Received: from mga01.intel.com ([192.55.52.88]:57995 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752825AbeGGOZ6 (ORCPT ); Sat, 7 Jul 2018 10:25:58 -0400 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Jul 2018 07:25:57 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,320,1526367600"; d="scan'208";a="214144901" Received: from lahna.fi.intel.com (HELO lahna) ([10.237.72.157]) by orsmga004.jf.intel.com with SMTP; 07 Jul 2018 07:25:53 -0700 Received: by lahna (sSMTP sendmail emulation); Sat, 07 Jul 2018 17:25:53 +0300 Date: Sat, 7 Jul 2018 17:25:53 +0300 From: Mika Westerberg To: Lukas Wunner Cc: linux-kernel@vger.kernel.org, Andreas Noever , Michael Jamet , Yehezkel Bernat , "Rafael J. Wysocki" , Christian Kellner , Mario Limonciello Subject: Re: [PATCH 5/5] thunderbolt: Add support for runtime PM Message-ID: <20180707142553.GP2534@lahna.fi.intel.com> References: <20180618110731.57427-1-mika.westerberg@linux.intel.com> <20180618110731.57427-6-mika.westerberg@linux.intel.com> <20180707133815.GA6656@wunner.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180707133815.GA6656@wunner.de> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo User-Agent: Mutt/1.10.0 (2018-05-17) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jul 07, 2018 at 03:38:15PM +0200, Lukas Wunner wrote: > On Mon, Jun 18, 2018 at 02:07:31PM +0300, Mika Westerberg wrote: > > --- a/drivers/thunderbolt/domain.c > > +++ b/drivers/thunderbolt/domain.c > > @@ -132,6 +133,8 @@ static ssize_t boot_acl_show(struct device *dev, struct device_attribute *attr, > > if (!uuids) > > return -ENOMEM; > > > > + pm_runtime_get_sync(&tb->dev); > > + > > if (mutex_lock_interruptible(&tb->lock)) { > > ret = -ERESTARTSYS; > > goto out; > [snip] > > @@ -426,6 +437,13 @@ int tb_domain_add(struct tb *tb) > > /* This starts event processing */ > > mutex_unlock(&tb->lock); > > > > + pm_runtime_no_callbacks(&tb->dev); > > + pm_runtime_set_active(&tb->dev); > > + pm_runtime_enable(&tb->dev); > > + pm_runtime_set_autosuspend_delay(&tb->dev, TB_AUTOSUSPEND_DELAY); > > + pm_runtime_mark_last_busy(&tb->dev); > > + pm_runtime_use_autosuspend(&tb->dev); > > + > > return 0; > > > > err_domain_del: > > You're setting pm_runtime_no_callbacks() on the domain. A side effect of > setting this flag is that whenever the domain's device is runtime resumed, > it's parent (the NHI) is *not* runtime resumed, see this comment in > rpm_resume(): > > /* > * See if we can skip waking up the parent. This is safe only if > * power.no_callbacks is set, because otherwise we don't know whether > * the resume will actually succeed. > */ > > Above, you're runtime resuming the domain in boot_acl_show(). So if the > NHI is runtime suspended while that sysfs attribute is accessed, it won't > be runtime resumed. Is that actually what you want? No, it should be runtime resumed when domain is. Looking at the code in question bit more deeper: /* * See if we can skip waking up the parent. This is safe only if * power.no_callbacks is set, because otherwise we don't know whether * the resume will actually succeed. */ if (dev->power.no_callbacks && !parent && dev->parent) { spin_lock_nested(&dev->parent->power.lock, SINGLE_DEPTH_NESTING); if (dev->parent->power.disable_depth > 0 || dev->parent->power.ignore_children || dev->parent->power.runtime_status == RPM_ACTIVE) { atomic_inc(&dev->parent->power.child_count); spin_unlock(&dev->parent->power.lock); retval = 1; goto no_callback; /* Assume success. */ } spin_unlock(&dev->parent->power.lock); } So skipping waking the parent can only happen if any of the following conditions are true: - Parent has runtime PM disabled - Parent has ignore_children set - Parent is already resumed As far I can tell there can't be situation you describe that the parent would not be runtime resumed when the domain is. > > @@ -514,6 +532,28 @@ void tb_domain_complete(struct tb *tb) > > tb->cm_ops->complete(tb); > > } > > > > +int tb_domain_runtime_suspend(struct tb *tb) > > +{ > > + if (tb->cm_ops->runtime_suspend) { > > + int ret = tb->cm_ops->runtime_suspend(tb); > > + if (ret) > > + return ret; > > + } > > + tb_ctl_stop(tb->ctl); > > + return 0; > > +} > > + > > +int tb_domain_runtime_resume(struct tb *tb) > > +{ > > + tb_ctl_start(tb->ctl); > > + if (tb->cm_ops->runtime_resume) { > > + int ret = tb->cm_ops->runtime_resume(tb); > > + if (ret) > > + return ret; > > + } > > + return 0; > > +} > > + > > /** > > * tb_domain_approve_switch() - Approve switch > > * @tb: Domain the switch belongs to > > --- a/drivers/thunderbolt/nhi.c > > +++ b/drivers/thunderbolt/nhi.c > > @@ -900,7 +900,32 @@ static void nhi_complete(struct device *dev) > > struct pci_dev *pdev = to_pci_dev(dev); > > struct tb *tb = pci_get_drvdata(pdev); > > > > - tb_domain_complete(tb); > > + /* > > + * If we were runtime suspended when system suspend started, > > + * schedule runtime resume now. It should bring the domain back > > + * to functional state. > > + */ > > + if (pm_runtime_suspended(&pdev->dev)) > > + pm_runtime_resume(&pdev->dev); > > + else > > + tb_domain_complete(tb); > > +} > > + > > +static int nhi_runtime_suspend(struct device *dev) > > +{ > > + struct pci_dev *pdev = to_pci_dev(dev); > > + struct tb *tb = pci_get_drvdata(pdev); > > + > > + return tb_domain_runtime_suspend(tb); > > +} > > + > > +static int nhi_runtime_resume(struct device *dev) > > +{ > > + struct pci_dev *pdev = to_pci_dev(dev); > > + struct tb *tb = pci_get_drvdata(pdev); > > + > > + nhi_enable_int_throttling(tb->nhi); > > + return tb_domain_runtime_resume(tb); > > } > > You're invoking tb_domain_runtime_suspend() from nhi_runtime_suspend(), > same for ->runtime_resume. > > Wouldn't it make more sense to make tb_domain_runtime_suspend() the > ->runtime_suspend callback of the domain instead of mixing it together > with NHI runtime suspend? You mean let the PM core to handle this for domain? Maybe but currently we do the same for other callbacks as well so this just follows that. > BTW, what's the purpose of nhi_enable_int_throttling()? It changes how fast interrupts get delivered and when to start throttling. Mostly needed in P2P functionality (but should not do any harm for control channel traffic). See also 8c6bba10fb92 ("thunderbolt: Configure interrupt throttling for all interrupts"). > > --- a/drivers/thunderbolt/switch.c > > +++ b/drivers/thunderbolt/switch.c > > +/* > > + * Currently only need to provide the callbacks. Everything else is handled > > + * in the connection manager. > > + */ > > +static int __maybe_unused tb_switch_runtime_suspend(struct device *dev) > > +{ > > + return 0; > > +} > > + > > +static int __maybe_unused tb_switch_runtime_resume(struct device *dev) > > +{ > > + return 0; > > +} > > + > > +static const struct dev_pm_ops tb_switch_pm_ops = { > > + SET_RUNTIME_PM_OPS(tb_switch_runtime_suspend, tb_switch_runtime_resume, > > + NULL) > > +}; > > + > > struct device_type tb_switch_type = { > > .name = "thunderbolt_device", > > .release = tb_switch_release, > > + .pm = &tb_switch_pm_ops, > > }; > > Looking at the call sites of RPM_GET_CALLBACK(), I'm under the impression > that if no callbacks are defined, the PM core will simply assume success. > Then you don't need to define any PM callbacks for tb_switch. Am I missing > something? If you don't define them, RPM_GET_CALLBACK() returns NULL and subsequent call to rpm_callback(NULL, dev) then returns -ENOSYS which is failure.