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?
> @@ -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?
BTW, what's the purpose of nhi_enable_int_throttling()?
> --- 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?
Thanks,
Lukas
