From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-16.8 required=3.0 tests=BAYES_00,
	HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT 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 AA83DC07E9B
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 01:36:30 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id 7BB7261106
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 01:36:30 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S230023AbhGUAzu (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Tue, 20 Jul 2021 20:55:50 -0400
Received: from szxga01-in.huawei.com ([45.249.212.187]:7040 "EHLO
        szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S229629AbhGUAzp (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 20 Jul 2021 20:55:45 -0400
Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.56])
        by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4GTyhB2GBJzYdZb;
        Wed, 21 Jul 2021 09:30:34 +0800 (CST)
Received: from dggpemm500004.china.huawei.com (7.185.36.219) by
 dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2176.2; Wed, 21 Jul 2021 09:36:20 +0800
Received: from huawei.com (10.174.28.241) by dggpemm500004.china.huawei.com
 (7.185.36.219) with Microsoft SMTP Server (version=TLS1_2,
 cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2176.2; Wed, 21 Jul
 2021 09:36:20 +0800
From:   Bixuan Cui <cuibixuan@huawei.com>
To:     <iommu@lists.linux-foundation.org>, <linux-kernel@vger.kernel.org>,
        <will@kernel.org>
CC:     <cuibixuan@huawei.com>, <weiyongjun1@huawei.com>,
        <john.wanghui@huawei.com>, <dingtianhong@huawei.com>,
        <thunder.leizhen@huawei.com>, <guohanjun@huawei.com>,
        <robin.murphy@arm.com>, <joro@8bytes.org>,
        <jean-philippe@linaro.org>, <Jonathan.Cameron@huawei.com>,
        <song.bao.hua@hisilicon.com>,
        <linux-arm-kernel@lists.infradead.org>
Subject: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
Date:   Wed, 21 Jul 2021 09:33:50 +0800
Message-ID: <20210721013350.17664-1-cuibixuan@huawei.com>
X-Mailer: git-send-email 2.17.1
MIME-Version: 1.0
Content-Type: text/plain
X-Originating-IP: [10.174.28.241]
X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To
 dggpemm500004.china.huawei.com (7.185.36.219)
X-CFilter-Loop: Reflected
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Add suspend and resume support for arm-smmu-v3 by low-power mode.

When the smmu is suspended, it is powered off and the registers are
cleared. So saves the msi_msg context during msi interrupt initialization
of smmu. When resume happens it calls arm_smmu_device_reset() to restore
the registers.

Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
---

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
 1 file changed, 64 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 235f9bdaeaf2..bf1163acbcb1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
 
 static bool disable_msipolling;
 module_param(disable_msipolling, bool, 0444);
+static bool bypass;
 MODULE_PARM_DESC(disable_msipolling,
 	"Disable MSI-based polling for CMD_SYNC completion.");
 
@@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
 	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
 	doorbell &= MSI_CFG0_ADDR_MASK;
 
+	/* Saves the msg context for resume if desc->msg is empty */
+	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
+		desc->msg.address_lo = msg->address_lo;
+		desc->msg.address_hi = msg->address_hi;
+		desc->msg.data = msg->data;
+	}
+
 	writeq_relaxed(doorbell, smmu->base + cfg[0]);
 	writel_relaxed(msg->data, smmu->base + cfg[1]);
 	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
 }
 
+static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
+{
+	struct msi_desc *desc;
+	struct device *dev = smmu->dev;
+
+	for_each_msi_entry(desc, dev) {
+		switch (desc->platform.msi_index) {
+		case EVTQ_MSI_INDEX:
+		case GERROR_MSI_INDEX:
+		case PRIQ_MSI_INDEX:
+			arm_smmu_write_msi_msg(desc, &(desc->msg));
+			break;
+		default:
+			continue;
+
+		}
+	}
+}
+
 static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 {
 	struct msi_desc *desc;
@@ -3184,11 +3211,17 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
 	devm_add_action(dev, arm_smmu_free_msis, dev);
 }
 
-static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
+static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu, bool resume_mode)
 {
 	int irq, ret;
 
-	arm_smmu_setup_msis(smmu);
+	if (!resume_mode)
+		arm_smmu_setup_msis(smmu);
+	else {
+		/* The irq doesn't need to be re-requested during resume */
+		arm_smmu_resume_msis(smmu);
+		return;
+	}
 
 	/* Request interrupt lines */
 	irq = smmu->evtq.q.irq;
@@ -3230,7 +3263,7 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
 	}
 }
 
-static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
+static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu, bool resume_mode)
 {
 	int ret, irq;
 	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
@@ -3257,7 +3290,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
 		if (ret < 0)
 			dev_warn(smmu->dev, "failed to enable combined irq\n");
 	} else
-		arm_smmu_setup_unique_irqs(smmu);
+		arm_smmu_setup_unique_irqs(smmu, resume_mode);
 
 	if (smmu->features & ARM_SMMU_FEAT_PRI)
 		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
@@ -3282,7 +3315,7 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
 	return ret;
 }
 
-static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
+static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume_mode)
 {
 	int ret;
 	u32 reg, enables;
@@ -3392,7 +3425,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
 		}
 	}
 
-	ret = arm_smmu_setup_irqs(smmu);
+	ret = arm_smmu_setup_irqs(smmu, resume_mode);
 	if (ret) {
 		dev_err(smmu->dev, "failed to setup irqs\n");
 		return ret;
@@ -3749,6 +3782,24 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
 	return devm_ioremap_resource(dev, &res);
 }
 
+static int __maybe_unused arm_smmu_suspend(struct device *dev)
+{
+	/*
+	 * The smmu is powered off and related registers are automatically
+	 * cleared when suspend. No need to do anything.
+	 */
+	return 0;
+}
+
+static int __maybe_unused arm_smmu_resume(struct device *dev)
+{
+	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+	arm_smmu_device_reset(smmu, true);
+
+	return 0;
+}
+
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -3756,7 +3807,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	resource_size_t ioaddr;
 	struct arm_smmu_device *smmu;
 	struct device *dev = &pdev->dev;
-	bool bypass;
 
 	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
 	if (!smmu)
@@ -3831,7 +3881,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, smmu);
 
 	/* Reset the device */
-	ret = arm_smmu_device_reset(smmu, bypass);
+	ret = arm_smmu_device_reset(smmu, false);
 	if (ret)
 		return ret;
 
@@ -3884,6 +3934,11 @@ static const struct of_device_id arm_smmu_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
 
+static const struct dev_pm_ops arm_smmu_pm_ops = {
+	.suspend = arm_smmu_suspend,
+	.resume = arm_smmu_resume,
+};
+
 static void arm_smmu_driver_unregister(struct platform_driver *drv)
 {
 	arm_smmu_sva_notifier_synchronize();
@@ -3895,6 +3950,7 @@ static struct platform_driver arm_smmu_driver = {
 		.name			= "arm-smmu-v3",
 		.of_match_table		= arm_smmu_of_match,
 		.suppress_bind_attrs	= true,
+		.pm			= &arm_smmu_pm_ops,
 	},
 	.probe	= arm_smmu_device_probe,
 	.remove	= arm_smmu_device_remove,
-- 
2.17.1


From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-15.4 required=3.0 tests=BAYES_00,
	HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1
	autolearn=unavailable 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 D2507C12002
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 11:56:08 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id AF513610F7
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 11:56:08 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S238810AbhGULPP (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Wed, 21 Jul 2021 07:15:15 -0400
Received: from foss.arm.com ([217.140.110.172]:52530 "EHLO foss.arm.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S239238AbhGULBp (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 21 Jul 2021 07:01:45 -0400
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
        by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 63E9E31B;
        Wed, 21 Jul 2021 04:42:22 -0700 (PDT)
Received: from [10.57.36.146] (unknown [10.57.36.146])
        by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2D1273F694;
        Wed, 21 Jul 2021 04:42:20 -0700 (PDT)
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
To:     Bixuan Cui <cuibixuan@huawei.com>,
        iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        will@kernel.org
Cc:     weiyongjun1@huawei.com, john.wanghui@huawei.com,
        dingtianhong@huawei.com, thunder.leizhen@huawei.com,
        guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org,
        Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com,
        linux-arm-kernel@lists.infradead.org, Marc Zyngier <maz@kernel.org>
References: <20210721013350.17664-1-cuibixuan@huawei.com>
From:   Robin Murphy <robin.murphy@arm.com>
Message-ID: <4e506481-5f6c-9c5e-eda3-300861581080@arm.com>
Date:   Wed, 21 Jul 2021 12:42:14 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20210721013350.17664-1-cuibixuan@huawei.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

[ +Marc for MSI bits ]

On 2021-07-21 02:33, Bixuan Cui wrote:
> Add suspend and resume support for arm-smmu-v3 by low-power mode.
> 
> When the smmu is suspended, it is powered off and the registers are
> cleared. So saves the msi_msg context during msi interrupt initialization
> of smmu. When resume happens it calls arm_smmu_device_reset() to restore
> the registers.
> 
> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
> Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
> Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
> Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> ---
> 
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
>   1 file changed, 64 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 235f9bdaeaf2..bf1163acbcb1 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
>   
>   static bool disable_msipolling;
>   module_param(disable_msipolling, bool, 0444);
> +static bool bypass;
>   MODULE_PARM_DESC(disable_msipolling,
>   	"Disable MSI-based polling for CMD_SYNC completion.");
>   
> @@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
>   	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>   	doorbell &= MSI_CFG0_ADDR_MASK;
>   
> +	/* Saves the msg context for resume if desc->msg is empty */
> +	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
> +		desc->msg.address_lo = msg->address_lo;
> +		desc->msg.address_hi = msg->address_hi;
> +		desc->msg.data = msg->data;
> +	}

My gut feeling is that this is something a device driver maybe shouldn't 
be poking into, but I'm not entirely familiar with the area :/

> +
>   	writeq_relaxed(doorbell, smmu->base + cfg[0]);
>   	writel_relaxed(msg->data, smmu->base + cfg[1]);
>   	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>   }
>   
> +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> +{
> +	struct msi_desc *desc;
> +	struct device *dev = smmu->dev;
> +
> +	for_each_msi_entry(desc, dev) {
> +		switch (desc->platform.msi_index) {
> +		case EVTQ_MSI_INDEX:
> +		case GERROR_MSI_INDEX:
> +		case PRIQ_MSI_INDEX:
> +			arm_smmu_write_msi_msg(desc, &(desc->msg));
> +			break;
> +		default:
> +			continue;
> +
> +		}
> +	}
> +}
> +
>   static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>   {
>   	struct msi_desc *desc;
> @@ -3184,11 +3211,17 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>   	devm_add_action(dev, arm_smmu_free_msis, dev);
>   }
>   
> -static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu, bool resume_mode)
>   {
>   	int irq, ret;
>   
> -	arm_smmu_setup_msis(smmu);
> +	if (!resume_mode)
> +		arm_smmu_setup_msis(smmu);
> +	else {
> +		/* The irq doesn't need to be re-requested during resume */
> +		arm_smmu_resume_msis(smmu);
> +		return;

What about wired IRQs?

> +	}
>   
>   	/* Request interrupt lines */
>   	irq = smmu->evtq.q.irq;
> @@ -3230,7 +3263,7 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>   	}
>   }
>   
> -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu, bool resume_mode)
>   {
>   	int ret, irq;
>   	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> @@ -3257,7 +3290,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>   		if (ret < 0)
>   			dev_warn(smmu->dev, "failed to enable combined irq\n");
>   	} else
> -		arm_smmu_setup_unique_irqs(smmu);
> +		arm_smmu_setup_unique_irqs(smmu, resume_mode);
>   
>   	if (smmu->features & ARM_SMMU_FEAT_PRI)
>   		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> @@ -3282,7 +3315,7 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
>   	return ret;
>   }
>   
> -static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> +static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume_mode)

Er, what about the use of "bypass" towards the end of the function. Have 
you even compiled this?

>   {
>   	int ret;
>   	u32 reg, enables;
> @@ -3392,7 +3425,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>   		}
>   	}
>   
> -	ret = arm_smmu_setup_irqs(smmu);
> +	ret = arm_smmu_setup_irqs(smmu, resume_mode);
>   	if (ret) {
>   		dev_err(smmu->dev, "failed to setup irqs\n");
>   		return ret;
> @@ -3749,6 +3782,24 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>   	return devm_ioremap_resource(dev, &res);
>   }
>   
> +static int __maybe_unused arm_smmu_suspend(struct device *dev)
> +{
> +	/*
> +	 * The smmu is powered off and related registers are automatically
> +	 * cleared when suspend. No need to do anything.
> +	 */

Is that guaranteed? What if suspend is only implemented by external 
clock-gating?

> +	return 0;
> +}
> +
> +static int __maybe_unused arm_smmu_resume(struct device *dev)
> +{
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> +	arm_smmu_device_reset(smmu, true);
> +
> +	return 0;
> +}
> +
>   static int arm_smmu_device_probe(struct platform_device *pdev)
>   {
>   	int irq, ret;
> @@ -3756,7 +3807,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	resource_size_t ioaddr;
>   	struct arm_smmu_device *smmu;
>   	struct device *dev = &pdev->dev;
> -	bool bypass;

Once again...

>   	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>   	if (!smmu)
> @@ -3831,7 +3881,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, smmu);
>   
>   	/* Reset the device */
> -	ret = arm_smmu_device_reset(smmu, bypass);

...either this is based on some out-of-tree hack which introduced its 
own uninitialised-usage bug here, or it doesn't even compile.

> +	ret = arm_smmu_device_reset(smmu, false);
>   	if (ret)
>   		return ret;
>   
> @@ -3884,6 +3934,11 @@ static const struct of_device_id arm_smmu_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>   
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> +	.suspend = arm_smmu_suspend,
> +	.resume = arm_smmu_resume,

Either use SET_SYSTEM_SLEEP_PM_OPS() here or drop the __maybe_unused 
annmotations above - they're pointless if the callbacks are referenced 
unconditionally.

Robin.

> +};
> +
>   static void arm_smmu_driver_unregister(struct platform_driver *drv)
>   {
>   	arm_smmu_sva_notifier_synchronize();
> @@ -3895,6 +3950,7 @@ static struct platform_driver arm_smmu_driver = {
>   		.name			= "arm-smmu-v3",
>   		.of_match_table		= arm_smmu_of_match,
>   		.suppress_bind_attrs	= true,
> +		.pm			= &arm_smmu_pm_ops,
>   	},
>   	.probe	= arm_smmu_device_probe,
>   	.remove	= arm_smmu_device_remove,
> 

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER,
	INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable 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 041B8C636CE
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 13:12:27 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id D87E561221
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 13:12:26 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S236312AbhGUMbs (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Wed, 21 Jul 2021 08:31:48 -0400
Received: from mail.kernel.org ([198.145.29.99]:60606 "EHLO mail.kernel.org"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S231680AbhGUMbm (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 21 Jul 2021 08:31:42 -0400
Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96])
        (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
        (No client certificate requested)
        by mail.kernel.org (Postfix) with ESMTPSA id 0A3C561029;
        Wed, 21 Jul 2021 13:12:19 +0000 (UTC)
Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org)
        by disco-boy.misterjones.org with esmtpsa  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
        (Exim 4.94.2)
        (envelope-from <maz@kernel.org>)
        id 1m6C1I-0003gp-V1; Wed, 21 Jul 2021 14:12:17 +0100
Date:   Wed, 21 Jul 2021 14:12:15 +0100
Message-ID: <878s1z3j68.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Robin Murphy <robin.murphy@arm.com>,
        Bixuan Cui <cuibixuan@huawei.com>
Cc:     iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        will@kernel.org, weiyongjun1@huawei.com, john.wanghui@huawei.com,
        dingtianhong@huawei.com, thunder.leizhen@huawei.com,
        guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org,
        Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
In-Reply-To: <4e506481-5f6c-9c5e-eda3-300861581080@arm.com>
References: <20210721013350.17664-1-cuibixuan@huawei.com>
        <4e506481-5f6c-9c5e-eda3-300861581080@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
X-SA-Exim-Connect-IP: 185.219.108.64
X-SA-Exim-Rcpt-To: robin.murphy@arm.com, cuibixuan@huawei.com, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, will@kernel.org, weiyongjun1@huawei.com, john.wanghui@huawei.com, dingtianhong@huawei.com, thunder.leizhen@huawei.com, guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org, Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com, linux-arm-kernel@lists.infradead.org
X-SA-Exim-Mail-From: maz@kernel.org
X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, 21 Jul 2021 12:42:14 +0100,
Robin Murphy <robin.murphy@arm.com> wrote:
> 
> [ +Marc for MSI bits ]
> 
> On 2021-07-21 02:33, Bixuan Cui wrote:
> > Add suspend and resume support for arm-smmu-v3 by low-power mode.
> > 
> > When the smmu is suspended, it is powered off and the registers are
> > cleared. So saves the msi_msg context during msi interrupt initialization
> > of smmu. When resume happens it calls arm_smmu_device_reset() to restore
> > the registers.
> > 
> > Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
> > Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
> > Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
> > Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
> > Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> > ---
> > 
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
> >   1 file changed, 64 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index 235f9bdaeaf2..bf1163acbcb1 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
> >     static bool disable_msipolling;
> >   module_param(disable_msipolling, bool, 0444);
> > +static bool bypass;
> >   MODULE_PARM_DESC(disable_msipolling,
> >   	"Disable MSI-based polling for CMD_SYNC completion.");
> >   @@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct
> > msi_desc *desc, struct msi_msg *msg)
> >   	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
> >   	doorbell &= MSI_CFG0_ADDR_MASK;
> >   +	/* Saves the msg context for resume if desc->msg is empty */
> > +	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
> > +		desc->msg.address_lo = msg->address_lo;
> > +		desc->msg.address_hi = msg->address_hi;
> > +		desc->msg.data = msg->data;
> > +	}
> 
> My gut feeling is that this is something a device driver maybe
> shouldn't be poking into, but I'm not entirely familiar with the area
> :/

Certainly not. If you rely on the message being stored into the
descriptors, then implement this in the core code, like we do for PCI.

> 
> > +
> >   	writeq_relaxed(doorbell, smmu->base + cfg[0]);
> >   	writel_relaxed(msg->data, smmu->base + cfg[1]);
> >   	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
> >   }
> >   +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> > +{
> > +	struct msi_desc *desc;
> > +	struct device *dev = smmu->dev;
> > +
> > +	for_each_msi_entry(desc, dev) {
> > +		switch (desc->platform.msi_index) {
> > +		case EVTQ_MSI_INDEX:
> > +		case GERROR_MSI_INDEX:
> > +		case PRIQ_MSI_INDEX:
> > +			arm_smmu_write_msi_msg(desc, &(desc->msg));

Consider using get_cached_msi_msg() instead of using the internals of
the descriptor.

> > +			break;
> > +		default:
> > +			continue;
> > +
> > +		}
> > +	}
> > +}
> > +
> >   static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
> >   {
> >   	struct msi_desc *desc;
> > @@ -3184,11 +3211,17 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
> >   	devm_add_action(dev, arm_smmu_free_msis, dev);
> >   }
> >   -static void arm_smmu_setup_unique_irqs(struct arm_smmu_device
> > *smmu)
> > +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu, bool resume_mode)
> >   {
> >   	int irq, ret;
> >   -	arm_smmu_setup_msis(smmu);
> > +	if (!resume_mode)
> > +		arm_smmu_setup_msis(smmu);
> > +	else {
> > +		/* The irq doesn't need to be re-requested during resume */
> > +		arm_smmu_resume_msis(smmu);
> > +		return;
> 
> What about wired IRQs?

Yeah. I assume the SMMU needs to be told which event gets signalled
using MSIs or IRQs? Or is that implied by the MSI being configured or
not (I used to know the answer to that, but I have long paged it out).

> 
> > +	}
> >     	/* Request interrupt lines */
> >   	irq = smmu->evtq.q.irq;
> > @@ -3230,7 +3263,7 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
> >   	}
> >   }
> >   -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> > +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu, bool resume_mode)
> >   {
> >   	int ret, irq;
> >   	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
> > @@ -3257,7 +3290,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
> >   		if (ret < 0)
> >   			dev_warn(smmu->dev, "failed to enable combined irq\n");
> >   	} else
> > -		arm_smmu_setup_unique_irqs(smmu);
> > +		arm_smmu_setup_unique_irqs(smmu, resume_mode);
> >     	if (smmu->features & ARM_SMMU_FEAT_PRI)
> >   		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
> > @@ -3282,7 +3315,7 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
> >   	return ret;
> >   }
> >   -static int arm_smmu_device_reset(struct arm_smmu_device *smmu,
> > bool bypass)
> > +static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume_mode)
> 
> Er, what about the use of "bypass" towards the end of the
> function. Have you even compiled this?

The author of the patch has conveniently made it a global value (see
line 3 of the patch). I'm sure it doesn't break anything... :-(

> 
> >   {
> >   	int ret;
> >   	u32 reg, enables;
> > @@ -3392,7 +3425,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
> >   		}
> >   	}
> >   -	ret = arm_smmu_setup_irqs(smmu);
> > +	ret = arm_smmu_setup_irqs(smmu, resume_mode);
> >   	if (ret) {
> >   		dev_err(smmu->dev, "failed to setup irqs\n");
> >   		return ret;
> > @@ -3749,6 +3782,24 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
> >   	return devm_ioremap_resource(dev, &res);
> >   }
> >   +static int __maybe_unused arm_smmu_suspend(struct device *dev)
> > +{
> > +	/*
> > +	 * The smmu is powered off and related registers are automatically
> > +	 * cleared when suspend. No need to do anything.
> > +	 */
> 
> Is that guaranteed? What if suspend is only implemented by external
> clock-gating?

+1. This seems awfully implementation/integration specific. I'd be
more in favour of a controlled teardown.

> 
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused arm_smmu_resume(struct device *dev)
> > +{
> > +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> > +
> > +	arm_smmu_device_reset(smmu, true);
> > +
> > +	return 0;
> > +}
> > +
> >   static int arm_smmu_device_probe(struct platform_device *pdev)
> >   {
> >   	int irq, ret;
> > @@ -3756,7 +3807,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >   	resource_size_t ioaddr;
> >   	struct arm_smmu_device *smmu;
> >   	struct device *dev = &pdev->dev;
> > -	bool bypass;
> 
> Once again...
> 
> >   	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
> >   	if (!smmu)
> > @@ -3831,7 +3881,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
> >   	platform_set_drvdata(pdev, smmu);
> >     	/* Reset the device */
> > -	ret = arm_smmu_device_reset(smmu, bypass);
> 
> ...either this is based on some out-of-tree hack which introduced its
> own uninitialised-usage bug here, or it doesn't even compile.
> 
> > +	ret = arm_smmu_device_reset(smmu, false);
> >   	if (ret)
> >   		return ret;
> >   @@ -3884,6 +3934,11 @@ static const struct of_device_id
> > arm_smmu_of_match[] = {
> >   };
> >   MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
> >   +static const struct dev_pm_ops arm_smmu_pm_ops = {
> > +	.suspend = arm_smmu_suspend,
> > +	.resume = arm_smmu_resume,
> 
> Either use SET_SYSTEM_SLEEP_PM_OPS() here or drop the __maybe_unused
> annmotations above - they're pointless if the callbacks are referenced
> unconditionally.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-15.4 required=3.0 tests=BAYES_00,
	HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1
	autolearn=unavailable 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 644D2C12002
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 14:00:23 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id 460C761287
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 14:00:23 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S232628AbhGUNTo (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Wed, 21 Jul 2021 09:19:44 -0400
Received: from foss.arm.com ([217.140.110.172]:55188 "EHLO foss.arm.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S239057AbhGUNTV (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 21 Jul 2021 09:19:21 -0400
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
        by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 564F31FB;
        Wed, 21 Jul 2021 06:59:56 -0700 (PDT)
Received: from [10.57.36.146] (unknown [10.57.36.146])
        by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2B6C13F73D;
        Wed, 21 Jul 2021 06:59:54 -0700 (PDT)
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
To:     Marc Zyngier <maz@kernel.org>, Bixuan Cui <cuibixuan@huawei.com>
Cc:     iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        will@kernel.org, weiyongjun1@huawei.com, john.wanghui@huawei.com,
        dingtianhong@huawei.com, thunder.leizhen@huawei.com,
        guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org,
        Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com,
        linux-arm-kernel@lists.infradead.org
References: <20210721013350.17664-1-cuibixuan@huawei.com>
 <4e506481-5f6c-9c5e-eda3-300861581080@arm.com> <878s1z3j68.wl-maz@kernel.org>
From:   Robin Murphy <robin.murphy@arm.com>
Message-ID: <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
Date:   Wed, 21 Jul 2021 14:59:47 +0100
User-Agent: Mozilla/5.0 (Windows NT 10.0; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <878s1z3j68.wl-maz@kernel.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021-07-21 14:12, Marc Zyngier wrote:
> On Wed, 21 Jul 2021 12:42:14 +0100,
> Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> [ +Marc for MSI bits ]
>>
>> On 2021-07-21 02:33, Bixuan Cui wrote:
>>> Add suspend and resume support for arm-smmu-v3 by low-power mode.
>>>
>>> When the smmu is suspended, it is powered off and the registers are
>>> cleared. So saves the msi_msg context during msi interrupt initialization
>>> of smmu. When resume happens it calls arm_smmu_device_reset() to restore
>>> the registers.
>>>
>>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
>>> Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
>>> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
>>> ---
>>>
>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
>>>    1 file changed, 64 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> index 235f9bdaeaf2..bf1163acbcb1 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
>>>      static bool disable_msipolling;
>>>    module_param(disable_msipolling, bool, 0444);
>>> +static bool bypass;
>>>    MODULE_PARM_DESC(disable_msipolling,
>>>    	"Disable MSI-based polling for CMD_SYNC completion.");
>>>    @@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct
>>> msi_desc *desc, struct msi_msg *msg)
>>>    	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>>>    	doorbell &= MSI_CFG0_ADDR_MASK;
>>>    +	/* Saves the msg context for resume if desc->msg is empty */
>>> +	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
>>> +		desc->msg.address_lo = msg->address_lo;
>>> +		desc->msg.address_hi = msg->address_hi;
>>> +		desc->msg.data = msg->data;
>>> +	}
>>
>> My gut feeling is that this is something a device driver maybe
>> shouldn't be poking into, but I'm not entirely familiar with the area
>> :/
> 
> Certainly not. If you rely on the message being stored into the
> descriptors, then implement this in the core code, like we do for PCI.

Ah, so it would be an acceptable compromise to *read* desc->msg (and 
thus avoid having to store our own copy of the message) if the core was 
guaranteed to cache it? That's good to know, thanks.

>>> +
>>>    	writeq_relaxed(doorbell, smmu->base + cfg[0]);
>>>    	writel_relaxed(msg->data, smmu->base + cfg[1]);
>>>    	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>>>    }
>>>    +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
>>> +{
>>> +	struct msi_desc *desc;
>>> +	struct device *dev = smmu->dev;
>>> +
>>> +	for_each_msi_entry(desc, dev) {
>>> +		switch (desc->platform.msi_index) {
>>> +		case EVTQ_MSI_INDEX:
>>> +		case GERROR_MSI_INDEX:
>>> +		case PRIQ_MSI_INDEX:
>>> +			arm_smmu_write_msi_msg(desc, &(desc->msg));
> 
> Consider using get_cached_msi_msg() instead of using the internals of
> the descriptor.

Oh, there's even a proper API for it, marvellous! I hadn't managed to 
dig that far myself :)

>>> +			break;
>>> +		default:
>>> +			continue;
>>> +
>>> +		}
>>> +	}
>>> +}
>>> +
>>>    static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>>    {
>>>    	struct msi_desc *desc;
>>> @@ -3184,11 +3211,17 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>>    	devm_add_action(dev, arm_smmu_free_msis, dev);
>>>    }
>>>    -static void arm_smmu_setup_unique_irqs(struct arm_smmu_device
>>> *smmu)
>>> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu, bool resume_mode)
>>>    {
>>>    	int irq, ret;
>>>    -	arm_smmu_setup_msis(smmu);
>>> +	if (!resume_mode)
>>> +		arm_smmu_setup_msis(smmu);
>>> +	else {
>>> +		/* The irq doesn't need to be re-requested during resume */
>>> +		arm_smmu_resume_msis(smmu);
>>> +		return;
>>
>> What about wired IRQs?
> 
> Yeah. I assume the SMMU needs to be told which event gets signalled
> using MSIs or IRQs? Or is that implied by the MSI being configured or
> not (I used to know the answer to that, but I have long paged it out).

My mistake there - I misread the rather convoluted existing flow to 
think that bailing here would fail to enable wired IRQs, but of course 
the signalling decision is based on whether the MSI address is non-zero, 
and the enables in ARM_SMMU_IRQ_CTRL still apply either way.

Given that, I think this is that point at which to refactor this whole 
part so that logically requesting and physically programming the 
interrupts are split into distinct stages, which can then be called 
seperately as appropriate. Personally I have a particular dislike of 
this general anti-pattern:

	int do_a_thing(bool but_only_do_part_of_it)

>>> +	}
>>>      	/* Request interrupt lines */
>>>    	irq = smmu->evtq.q.irq;
>>> @@ -3230,7 +3263,7 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>>>    	}
>>>    }
>>>    -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu, bool resume_mode)
>>>    {
>>>    	int ret, irq;
>>>    	u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>>> @@ -3257,7 +3290,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>>    		if (ret < 0)
>>>    			dev_warn(smmu->dev, "failed to enable combined irq\n");
>>>    	} else
>>> -		arm_smmu_setup_unique_irqs(smmu);
>>> +		arm_smmu_setup_unique_irqs(smmu, resume_mode);
>>>      	if (smmu->features & ARM_SMMU_FEAT_PRI)
>>>    		irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
>>> @@ -3282,7 +3315,7 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
>>>    	return ret;
>>>    }
>>>    -static int arm_smmu_device_reset(struct arm_smmu_device *smmu,
>>> bool bypass)
>>> +static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume_mode)
>>
>> Er, what about the use of "bypass" towards the end of the
>> function. Have you even compiled this?
> 
> The author of the patch has conveniently made it a global value (see
> line 3 of the patch). I'm sure it doesn't break anything... :-(

Oh blimey, I hadn't even turned on my "things that make no flippin' 
sense" radar to notice that, thanks for the correction. Indeed that's 
pretty busted for SMMU instances probing concurrently.

As with interrupts, reset probably wants a bit of refactoring to 
separate out the probe-time concerns of cleaning up any left-over 
configuration from firmware/kdump/whatever, and the act of putting the 
hardware into the operational state we want. For instance, if it happens 
that state is *not* lost on suspend, subsequently warning about 
CR0_SMMUEN being enabled on resume would be bad.

>>>    {
>>>    	int ret;
>>>    	u32 reg, enables;
>>> @@ -3392,7 +3425,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>>>    		}
>>>    	}
>>>    -	ret = arm_smmu_setup_irqs(smmu);
>>> +	ret = arm_smmu_setup_irqs(smmu, resume_mode);
>>>    	if (ret) {
>>>    		dev_err(smmu->dev, "failed to setup irqs\n");
>>>    		return ret;
>>> @@ -3749,6 +3782,24 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>>>    	return devm_ioremap_resource(dev, &res);
>>>    }
>>>    +static int __maybe_unused arm_smmu_suspend(struct device *dev)
>>> +{
>>> +	/*
>>> +	 * The smmu is powered off and related registers are automatically
>>> +	 * cleared when suspend. No need to do anything.
>>> +	 */
>>
>> Is that guaranteed? What if suspend is only implemented by external
>> clock-gating?
> 
> +1. This seems awfully implementation/integration specific. I'd be
> more in favour of a controlled teardown.
> 
>>
>>> +	return 0;
>>> +}
>>> +
>>> +static int __maybe_unused arm_smmu_resume(struct device *dev)
>>> +{
>>> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>>> +
>>> +	arm_smmu_device_reset(smmu, true);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>    {
>>>    	int irq, ret;
>>> @@ -3756,7 +3807,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>    	resource_size_t ioaddr;
>>>    	struct arm_smmu_device *smmu;
>>>    	struct device *dev = &pdev->dev;
>>> -	bool bypass;
>>
>> Once again...
>>
>>>    	smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>>>    	if (!smmu)
>>> @@ -3831,7 +3881,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>    	platform_set_drvdata(pdev, smmu);
>>>      	/* Reset the device */
>>> -	ret = arm_smmu_device_reset(smmu, bypass);
>>
>> ...either this is based on some out-of-tree hack which introduced its
>> own uninitialised-usage bug here, or it doesn't even compile.

As above, apologies for the oversight there. It's not as badly wrong as 
I thought, but it's still not right.

Thanks,
Robin.

>>
>>> +	ret = arm_smmu_device_reset(smmu, false);
>>>    	if (ret)
>>>    		return ret;
>>>    @@ -3884,6 +3934,11 @@ static const struct of_device_id
>>> arm_smmu_of_match[] = {
>>>    };
>>>    MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>    +static const struct dev_pm_ops arm_smmu_pm_ops = {
>>> +	.suspend = arm_smmu_suspend,
>>> +	.resume = arm_smmu_resume,
>>
>> Either use SET_SYSTEM_SLEEP_PM_OPS() here or drop the __maybe_unused
>> annmotations above - they're pointless if the callbacks are referenced
>> unconditionally.
> 
> Thanks,
> 
> 	M.
> 

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER,
	INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable 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 4C9C6C636C9
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 15:04:14 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id 2BD5761246
	for <linux-kernel@archiver.kernel.org>; Wed, 21 Jul 2021 15:04:14 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S239522AbhGUOXf (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Wed, 21 Jul 2021 10:23:35 -0400
Received: from mail.kernel.org ([198.145.29.99]:56894 "EHLO mail.kernel.org"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S239491AbhGUOUu (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Wed, 21 Jul 2021 10:20:50 -0400
Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96])
        (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
        (No client certificate requested)
        by mail.kernel.org (Postfix) with ESMTPSA id 2D6A16120E;
        Wed, 21 Jul 2021 15:01:27 +0000 (UTC)
Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org)
        by disco-boy.misterjones.org with esmtpsa  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
        (Exim 4.94.2)
        (envelope-from <maz@kernel.org>)
        id 1m6Div-0005Ox-3p; Wed, 21 Jul 2021 16:01:25 +0100
Date:   Wed, 21 Jul 2021 16:01:24 +0100
Message-ID: <877dhj3e4b.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Robin Murphy <robin.murphy@arm.com>
Cc:     Bixuan Cui <cuibixuan@huawei.com>,
        iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org,
        will@kernel.org, weiyongjun1@huawei.com, john.wanghui@huawei.com,
        dingtianhong@huawei.com, thunder.leizhen@huawei.com,
        guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org,
        Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
In-Reply-To: <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
References: <20210721013350.17664-1-cuibixuan@huawei.com>
        <4e506481-5f6c-9c5e-eda3-300861581080@arm.com>
        <878s1z3j68.wl-maz@kernel.org>
        <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
X-SA-Exim-Connect-IP: 185.219.108.64
X-SA-Exim-Rcpt-To: robin.murphy@arm.com, cuibixuan@huawei.com, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, will@kernel.org, weiyongjun1@huawei.com, john.wanghui@huawei.com, dingtianhong@huawei.com, thunder.leizhen@huawei.com, guohanjun@huawei.com, joro@8bytes.org, jean-philippe@linaro.org, Jonathan.Cameron@huawei.com, song.bao.hua@hisilicon.com, linux-arm-kernel@lists.infradead.org
X-SA-Exim-Mail-From: maz@kernel.org
X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, 21 Jul 2021 14:59:47 +0100,
Robin Murphy <robin.murphy@arm.com> wrote:
> 
> On 2021-07-21 14:12, Marc Zyngier wrote:
> > On Wed, 21 Jul 2021 12:42:14 +0100,
> > Robin Murphy <robin.murphy@arm.com> wrote:
> >> 
> >> [ +Marc for MSI bits ]
> >> 
> >> On 2021-07-21 02:33, Bixuan Cui wrote:
> >>> Add suspend and resume support for arm-smmu-v3 by low-power mode.
> >>> 
> >>> When the smmu is suspended, it is powered off and the registers are
> >>> cleared. So saves the msi_msg context during msi interrupt initialization
> >>> of smmu. When resume happens it calls arm_smmu_device_reset() to restore
> >>> the registers.
> >>> 
> >>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
> >>> Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
> >>> Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
> >>> Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
> >>> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
> >>> ---
> >>> 
> >>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
> >>>    1 file changed, 64 insertions(+), 8 deletions(-)
> >>> 
> >>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> index 235f9bdaeaf2..bf1163acbcb1 100644
> >>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
> >>>      static bool disable_msipolling;
> >>>    module_param(disable_msipolling, bool, 0444);
> >>> +static bool bypass;
> >>>    MODULE_PARM_DESC(disable_msipolling,
> >>>    	"Disable MSI-based polling for CMD_SYNC completion.");
> >>>    @@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct
> >>> msi_desc *desc, struct msi_msg *msg)
> >>>    	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
> >>>    	doorbell &= MSI_CFG0_ADDR_MASK;
> >>>    +	/* Saves the msg context for resume if desc->msg is empty */
> >>> +	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
> >>> +		desc->msg.address_lo = msg->address_lo;
> >>> +		desc->msg.address_hi = msg->address_hi;
> >>> +		desc->msg.data = msg->data;
> >>> +	}
> >> 
> >> My gut feeling is that this is something a device driver maybe
> >> shouldn't be poking into, but I'm not entirely familiar with the area
> >> :/
> > 
> > Certainly not. If you rely on the message being stored into the
> > descriptors, then implement this in the core code, like we do for PCI.
> 
> Ah, so it would be an acceptable compromise to *read* desc->msg (and
> thus avoid having to store our own copy of the message) if the core
> was guaranteed to cache it? That's good to know, thanks.

Yeah, vfio, a couple of other weird drivers and (*surprise!*) ia64 are
using this kind of trick. I don't see a reason not to implement that
for platform-MSI (although level signalling may be interesting...), or
even to move it into the core MSI code.

> 
> >>> +
> >>>    	writeq_relaxed(doorbell, smmu->base + cfg[0]);
> >>>    	writel_relaxed(msg->data, smmu->base + cfg[1]);
> >>>    	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
> >>>    }
> >>>    +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
> >>> +{
> >>> +	struct msi_desc *desc;
> >>> +	struct device *dev = smmu->dev;
> >>> +
> >>> +	for_each_msi_entry(desc, dev) {
> >>> +		switch (desc->platform.msi_index) {
> >>> +		case EVTQ_MSI_INDEX:
> >>> +		case GERROR_MSI_INDEX:
> >>> +		case PRIQ_MSI_INDEX:
> >>> +			arm_smmu_write_msi_msg(desc, &(desc->msg));
> > 
> > Consider using get_cached_msi_msg() instead of using the internals of
> > the descriptor.
> 
> Oh, there's even a proper API for it, marvellous! I hadn't managed to
> dig that far myself :)

It is a bit odd in the sense that it takes a copy of the message
instead of returning a pointer, but at least this solves lifetime
issues.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-15.5 required=3.0 tests=BAYES_00,
	HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,
	USER_AGENT_SANE_1 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 5D7C6C63793
	for <linux-kernel@archiver.kernel.org>; Thu, 22 Jul 2021 06:34:39 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id 400C56127C
	for <linux-kernel@archiver.kernel.org>; Thu, 22 Jul 2021 06:34:39 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S231324AbhGVFyC (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Thu, 22 Jul 2021 01:54:02 -0400
Received: from szxga08-in.huawei.com ([45.249.212.255]:12235 "EHLO
        szxga08-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S230123AbhGVFyB (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 22 Jul 2021 01:54:01 -0400
Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.55])
        by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4GVjFp12yFz1CM75;
        Thu, 22 Jul 2021 14:28:46 +0800 (CST)
Received: from dggpemm500004.china.huawei.com (7.185.36.219) by
 dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2176.2; Thu, 22 Jul 2021 14:34:33 +0800
Received: from [10.174.177.91] (10.174.177.91) by
 dggpemm500004.china.huawei.com (7.185.36.219) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2176.2; Thu, 22 Jul 2021 14:34:33 +0800
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
To:     Marc Zyngier <maz@kernel.org>, Robin Murphy <robin.murphy@arm.com>
CC:     <iommu@lists.linux-foundation.org>, <linux-kernel@vger.kernel.org>,
        <will@kernel.org>, <weiyongjun1@huawei.com>,
        <john.wanghui@huawei.com>, <dingtianhong@huawei.com>,
        <thunder.leizhen@huawei.com>, <guohanjun@huawei.com>,
        <joro@8bytes.org>, <jean-philippe@linaro.org>,
        <Jonathan.Cameron@huawei.com>, <song.bao.hua@hisilicon.com>,
        <linux-arm-kernel@lists.infradead.org>
References: <20210721013350.17664-1-cuibixuan@huawei.com>
 <4e506481-5f6c-9c5e-eda3-300861581080@arm.com> <878s1z3j68.wl-maz@kernel.org>
 <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com> <877dhj3e4b.wl-maz@kernel.org>
From:   Bixuan Cui <cuibixuan@huawei.com>
Message-ID: <5054a5cd-579f-3fe9-1884-5219c8d13531@huawei.com>
Date:   Thu, 22 Jul 2021 14:34:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <877dhj3e4b.wl-maz@kernel.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.174.177.91]
X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To
 dggpemm500004.china.huawei.com (7.185.36.219)
X-CFilter-Loop: Reflected
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 2021/7/21 23:01, Marc Zyngier wrote:
> On Wed, 21 Jul 2021 14:59:47 +0100,
> Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-07-21 14:12, Marc Zyngier wrote:
>>> On Wed, 21 Jul 2021 12:42:14 +0100,
>>> Robin Murphy <robin.murphy@arm.com> wrote:
>>>>
>>>> [ +Marc for MSI bits ]
>>>>
>>>> On 2021-07-21 02:33, Bixuan Cui wrote:
>>>>> Add suspend and resume support for arm-smmu-v3 by low-power mode.
>>>>>
>>>>> When the smmu is suspended, it is powered off and the registers are
>>>>> cleared. So saves the msi_msg context during msi interrupt initialization
>>>>> of smmu. When resume happens it calls arm_smmu_device_reset() to restore
>>>>> the registers.
>>>>>
>>>>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
>>>>> Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>>> Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>> Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
>>>>> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
>>>>> ---
>>>>>
>>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
>>>>>    1 file changed, 64 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> index 235f9bdaeaf2..bf1163acbcb1 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>>> @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
>>>>>      static bool disable_msipolling;
>>>>>    module_param(disable_msipolling, bool, 0444);
>>>>> +static bool bypass;
>>>>>    MODULE_PARM_DESC(disable_msipolling,
>>>>>    	"Disable MSI-based polling for CMD_SYNC completion.");
>>>>>    @@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct
>>>>> msi_desc *desc, struct msi_msg *msg)
>>>>>    	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>>>>>    	doorbell &= MSI_CFG0_ADDR_MASK;
>>>>>    +	/* Saves the msg context for resume if desc->msg is empty */
>>>>> +	if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
>>>>> +		desc->msg.address_lo = msg->address_lo;
>>>>> +		desc->msg.address_hi = msg->address_hi;
>>>>> +		desc->msg.data = msg->data;
>>>>> +	}
>>>>
>>>> My gut feeling is that this is something a device driver maybe
>>>> shouldn't be poking into, but I'm not entirely familiar with the area
>>>> :/
>>>
>>> Certainly not. If you rely on the message being stored into the
>>> descriptors, then implement this in the core code, like we do for PCI.
>>
>> Ah, so it would be an acceptable compromise to *read* desc->msg (and
>> thus avoid having to store our own copy of the message) if the core
>> was guaranteed to cache it? That's good to know, thanks.
> 
> Yeah, vfio, a couple of other weird drivers and (*surprise!*) ia64 are
> using this kind of trick. I don't see a reason not to implement that
> for platform-MSI (although level signalling may be interesting...), or
> even to move it into the core MSI code.
Agree. If msg is saved to desc->msg in MSI core, the code here will not need.
During the initialization of the MSI interrupt of the SMMU, the desc->msg
is never used. So I save msg to desc->msg for resume use.


>>
>>>>> +
>>>>>    	writeq_relaxed(doorbell, smmu->base + cfg[0]);
>>>>>    	writel_relaxed(msg->data, smmu->base + cfg[1]);
>>>>>    	writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>>>>>    }
>>>>>    +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
>>>>> +{
>>>>> +	struct msi_desc *desc;
>>>>> +	struct device *dev = smmu->dev;
>>>>> +
>>>>> +	for_each_msi_entry(desc, dev) {
>>>>> +		switch (desc->platform.msi_index) {
>>>>> +		case EVTQ_MSI_INDEX:
>>>>> +		case GERROR_MSI_INDEX:
>>>>> +		case PRIQ_MSI_INDEX:
>>>>> +			arm_smmu_write_msi_msg(desc, &(desc->msg));
>>>
>>> Consider using get_cached_msi_msg() instead of using the internals of
>>> the descriptor.
>>
>> Oh, there's even a proper API for it, marvellous! I hadn't managed to
>> dig that far myself :)
> 
> It is a bit odd in the sense that it takes a copy of the message
> instead of returning a pointer, but at least this solves lifetime
> issues.
The code of arm_smmu_write_msi_msg() is multiplexed to restore the register. Therefore,
the parameter must be supplemented. Generally, desc is sufficient as an input parameter..
:)

Thanks,
Bixuan Cui
> 
> Thanks,
> 
> 	M.
> 

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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=-15.5 required=3.0 tests=BAYES_00,
	HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,
	USER_AGENT_SANE_1 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 06427C63797
	for <linux-kernel@archiver.kernel.org>; Thu, 22 Jul 2021 07:27:03 +0000 (UTC)
Received: from vger.kernel.org (vger.kernel.org [23.128.96.18])
	by mail.kernel.org (Postfix) with ESMTP id DD94C60551
	for <linux-kernel@archiver.kernel.org>; Thu, 22 Jul 2021 07:27:02 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S230427AbhGVGq0 (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Thu, 22 Jul 2021 02:46:26 -0400
Received: from szxga01-in.huawei.com ([45.249.212.187]:15046 "EHLO
        szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S229955AbhGVGqX (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 22 Jul 2021 02:46:23 -0400
Received: from dggemv703-chm.china.huawei.com (unknown [172.30.72.57])
        by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4GVkT06F5CzZrdy;
        Thu, 22 Jul 2021 15:23:32 +0800 (CST)
Received: from dggpemm500004.china.huawei.com (7.185.36.219) by
 dggemv703-chm.china.huawei.com (10.3.19.46) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2176.2; Thu, 22 Jul 2021 15:26:52 +0800
Received: from [10.174.177.91] (10.174.177.91) by
 dggpemm500004.china.huawei.com (7.185.36.219) with Microsoft SMTP Server
 (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
 15.1.2176.2; Thu, 22 Jul 2021 15:26:51 +0800
Subject: Re: [PATCH -next] iommu/arm-smmu-v3: Add suspend and resume support
To:     Robin Murphy <robin.murphy@arm.com>, Marc Zyngier <maz@kernel.org>
CC:     <iommu@lists.linux-foundation.org>, <linux-kernel@vger.kernel.org>,
        <will@kernel.org>, <weiyongjun1@huawei.com>,
        <john.wanghui@huawei.com>, <dingtianhong@huawei.com>,
        <thunder.leizhen@huawei.com>, <guohanjun@huawei.com>,
        <joro@8bytes.org>, <jean-philippe@linaro.org>,
        <Jonathan.Cameron@huawei.com>, <song.bao.hua@hisilicon.com>,
        <linux-arm-kernel@lists.infradead.org>
References: <20210721013350.17664-1-cuibixuan@huawei.com>
 <4e506481-5f6c-9c5e-eda3-300861581080@arm.com> <878s1z3j68.wl-maz@kernel.org>
 <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
From:   Bixuan Cui <cuibixuan@huawei.com>
Message-ID: <9c268adf-5d11-1854-583f-c9e88636cf09@huawei.com>
Date:   Thu, 22 Jul 2021 15:26:50 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <848befb0-7a9a-0b2b-8be9-3dfa02919488@arm.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-Originating-IP: [10.174.177.91]
X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To
 dggpemm500004.china.huawei.com (7.185.36.219)
X-CFilter-Loop: Reflected
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 2021/7/21 21:59, Robin Murphy wrote:
>> On Wed, 21 Jul 2021 12:42:14 +0100,
>> Robin Murphy <robin.murphy@arm.com> wrote:
>>>
>>> [ +Marc for MSI bits ]
>>>
>>> On 2021-07-21 02:33, Bixuan Cui wrote:
>>>> Add suspend and resume support for arm-smmu-v3 by low-power mode.
>>>>
>>>> When the smmu is suspended, it is powered off and the registers are
>>>> cleared. So saves the msi_msg context during msi interrupt initialization
>>>> of smmu. When resume happens it calls arm_smmu_device_reset() to restore
>>>> the registers.
>>>>
>>>> Signed-off-by: Bixuan Cui <cuibixuan@huawei.com>
>>>> Reviewed-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>> Reviewed-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> Reviewed-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> Reviewed-by: Hanjun Guo <guohanjun@huawei.com>
>>>> ---
>>>>
>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 72 ++++++++++++++++++---
>>>>    1 file changed, 64 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> index 235f9bdaeaf2..bf1163acbcb1 100644
>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> @@ -40,6 +40,7 @@ MODULE_PARM_DESC(disable_bypass,
>>>>      static bool disable_msipolling;
>>>>    module_param(disable_msipolling, bool, 0444);
>>>> +static bool bypass;
>>>>    MODULE_PARM_DESC(disable_msipolling,
>>>>        "Disable MSI-based polling for CMD_SYNC completion.");
>>>>    @@ -3129,11 +3130,37 @@ static void arm_smmu_write_msi_msg(struct
>>>> msi_desc *desc, struct msi_msg *msg)
>>>>        doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
>>>>        doorbell &= MSI_CFG0_ADDR_MASK;
>>>>    +    /* Saves the msg context for resume if desc->msg is empty */
>>>> +    if (desc->msg.address_lo == 0 && desc->msg.address_hi == 0) {
>>>> +        desc->msg.address_lo = msg->address_lo;
>>>> +        desc->msg.address_hi = msg->address_hi;
>>>> +        desc->msg.data = msg->data;
>>>> +    }
>>>
>>> My gut feeling is that this is something a device driver maybe
>>> shouldn't be poking into, but I'm not entirely familiar with the area
>>> :/
>>
>> Certainly not. If you rely on the message being stored into the
>> descriptors, then implement this in the core code, like we do for PCI.
> 
> Ah, so it would be an acceptable compromise to *read* desc->msg (and thus avoid having to store our own copy of the message) if the core was guaranteed to cache it? That's good to know, thanks.
> 
>>>> +
>>>>        writeq_relaxed(doorbell, smmu->base + cfg[0]);
>>>>        writel_relaxed(msg->data, smmu->base + cfg[1]);
>>>>        writel_relaxed(ARM_SMMU_MEMATTR_DEVICE_nGnRE, smmu->base + cfg[2]);
>>>>    }
>>>>    +static void arm_smmu_resume_msis(struct arm_smmu_device *smmu)
>>>> +{
>>>> +    struct msi_desc *desc;
>>>> +    struct device *dev = smmu->dev;
>>>> +
>>>> +    for_each_msi_entry(desc, dev) {
>>>> +        switch (desc->platform.msi_index) {
>>>> +        case EVTQ_MSI_INDEX:
>>>> +        case GERROR_MSI_INDEX:
>>>> +        case PRIQ_MSI_INDEX:
>>>> +            arm_smmu_write_msi_msg(desc, &(desc->msg));
>>
>> Consider using get_cached_msi_msg() instead of using the internals of
>> the descriptor.
> 
> Oh, there's even a proper API for it, marvellous! I hadn't managed to dig that far myself :)
Thanks for your suggestion, I'll use get_cached_msi_msg() instead.

> 
>>>> +            break;
>>>> +        default:
>>>> +            continue;
>>>> +
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>>    static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>>>    {
>>>>        struct msi_desc *desc;
>>>> @@ -3184,11 +3211,17 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>>>>        devm_add_action(dev, arm_smmu_free_msis, dev);
>>>>    }
>>>>    -static void arm_smmu_setup_unique_irqs(struct arm_smmu_device
>>>> *smmu)
>>>> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu, bool resume_mode)
>>>>    {
>>>>        int irq, ret;
>>>>    -    arm_smmu_setup_msis(smmu);
>>>> +    if (!resume_mode)
>>>> +        arm_smmu_setup_msis(smmu);
>>>> +    else {
>>>> +        /* The irq doesn't need to be re-requested during resume */
>>>> +        arm_smmu_resume_msis(smmu);
>>>> +        return;
>>>
>>> What about wired IRQs?
>>
>> Yeah. I assume the SMMU needs to be told which event gets signalled
>> using MSIs or IRQs? Or is that implied by the MSI being configured or
>> not (I used to know the answer to that, but I have long paged it out).
> 
> My mistake there - I misread the rather convoluted existing flow to think that bailing here would fail to enable wired IRQs, but of course the signalling decision is based on whether the MSI address is non-zero, and the enables in ARM_SMMU_IRQ_CTRL still apply either way>
> Given that, I think this is that point at which to refactor this whole part so that logically requesting and physically programming the interrupts are split into distinct stages, which can then be called seperately as appropriate. Personally I have a particular dislike of this general anti-patten
> 
>     int do_a_thing(bool but_only_do_part_of_it)
I have verified this patch on the board in arm64, and the MSI interrupt can work properly after resume.
The important work is to restore ARM_SMMU_EVTQ_IRQ_CFGn/ARM_SMMU_GERROR_IRQ_CFGn/ARM_SMMU_PRIQ_IRQ_CFG0 register after the resume.

When adding the suspend/resume, I try to change the original code as little as possible.

Thanks
Bixuan Cui
> 
>>>> +    }
>>>>          /* Request interrupt lines */
>>>>        irq = smmu->evtq.q.irq;
>>>> @@ -3230,7 +3263,7 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>>>>        }
>>>>    }
>>>>    -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>>> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu, bool resume_mode)
>>>>    {
>>>>        int ret, irq;
>>>>        u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>>>> @@ -3257,7 +3290,7 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>>>>            if (ret < 0)
>>>>                dev_warn(smmu->dev, "failed to enable combined irq\n");
>>>>        } else
>>>> -        arm_smmu_setup_unique_irqs(smmu);
>>>> +        arm_smmu_setup_unique_irqs(smmu, resume_mode);
>>>>          if (smmu->features & ARM_SMMU_FEAT_PRI)
>>>>            irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
>>>> @@ -3282,7 +3315,7 @@ static int arm_smmu_device_disable(struct arm_smmu_device *smmu)
>>>>        return ret;
>>>>    }
>>>>    -static int arm_smmu_device_reset(struct arm_smmu_device *smmu,
>>>> bool bypass)
>>>> +static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool resume_mode)
>>>
>>> Er, what about the use of "bypass" towards the end of the
>>> function. Have you even compiled this?
>>
>> The author of the patch has conveniently made it a global value (see
>> line 3 of the patch). I'm sure it doesn't break anything... :-(
> 
> Oh blimey, I hadn't even turned on my "things that make no flippin' sense" radar to notice that, thanks for the correction. Indeed that's pretty busted for SMMU instances probing concurrently.
> 
> As with interrupts, reset probably wants a bit of refactoring to separate out the probe-time concerns of cleaning up any left-over configuration from firmware/kdump/whatever, and the act of putting the hardware into the operational state we want. For instance, if it happens that state is *not* lost on suspend, subsequently warning about CR0_SMMUEN being enabled on resume would be bad.
> 
>>>>    {
>>>>        int ret;
>>>>        u32 reg, enables;
>>>> @@ -3392,7 +3425,7 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu, bool bypass)
>>>>            }
>>>>        }
>>>>    -    ret = arm_smmu_setup_irqs(smmu);
>>>> +    ret = arm_smmu_setup_irqs(smmu, resume_mode);
>>>>        if (ret) {
>>>>            dev_err(smmu->dev, "failed to setup irqs\n");
>>>>            return ret;
>>>> @@ -3749,6 +3782,24 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>>>>        return devm_ioremap_resource(dev, &res);
>>>>    }
>>>>    +static int __maybe_unused arm_smmu_suspend(struct device *dev)
>>>> +{
>>>> +    /*
>>>> +     * The smmu is powered off and related registers are automatically
>>>> +     * cleared when suspend. No need to do anything.
>>>> +     */
>>>
>>> Is that guaranteed? What if suspend is only implemented by external
>>> clock-gating?
>>
>> +1. This seems awfully implementation/integration specific. I'd be
>> more in favour of a controlled teardown.
Indeed, The current code applies to the scenario where the SMMU is directly powered off.
I'm not sure what other implementation/integration are. :(

>>
>>>
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int __maybe_unused arm_smmu_resume(struct device *dev)
>>>> +{
>>>> +    struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>>>> +
>>>> +    arm_smmu_device_reset(smmu, true);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>    {
>>>>        int irq, ret;
>>>> @@ -3756,7 +3807,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>        resource_size_t ioaddr;
>>>>        struct arm_smmu_device *smmu;
>>>>        struct device *dev = &pdev->dev;
>>>> -    bool bypass;
>>>
>>> Once again...
>>>
>>>>        smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
>>>>        if (!smmu)
>>>> @@ -3831,7 +3881,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>        platform_set_drvdata(pdev, smmu);
>>>>          /* Reset the device */
>>>> -    ret = arm_smmu_device_reset(smmu, bypass);
>>>
>>> ...either this is based on some out-of-tree hack which introduced its
>>> own uninitialised-usage bug here, or it doesn't even compile.
> 
> As above, apologies for the oversight there. It's not as badly wrong as I thought, but it's still not right.
> 
> Thanks,
> Robin.
> 
>>>
>>>> +    ret = arm_smmu_device_reset(smmu, false);
>>>>        if (ret)
>>>>            return ret;
>>>>    @@ -3884,6 +3934,11 @@ static const struct of_device_id
>>>> arm_smmu_of_match[] = {
>>>>    };
>>>>    MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>>    +static const struct dev_pm_ops arm_smmu_pm_ops = {
>>>> +    .suspend = arm_smmu_suspend,
>>>> +    .resume = arm_smmu_resume,
>>>
>>> Either use SET_SYSTEM_SLEEP_PM_OPS() here or drop the __maybe_unused
>>> annmotations above - they're pointless if the callbacks are referenced
>>> unconditionally.
Thank you for the advice here.



Thanks,
Bixuan Cui

>>
>> Thanks,
>>
>>     M.
>>
> .

