Re: [PATCH -next v2] iommu/arm-smmu-v3: Add suspend and resume support
From: Robin Murphy
Date: Tue Jul 27 2021 - 09:44:46 EST
On 2021-07-27 13:14, 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@xxxxxxxxxx>
Reviewed-by: Wei Yongjun <weiyongjun1@xxxxxxxxxx>
Reviewed-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
Reviewed-by: Ding Tianhong <dingtianhong@xxxxxxxxxx>
Reviewed-by: Hanjun Guo <guohanjun@xxxxxxxxxx>
---
Changes in v2:
* Using get_cached_msi_msg() instead of the descriptor to resume msi_msg
in arm_smmu_resume_msis();
* Move arm_smmu_resume_msis() from arm_smmu_setup_unique_irqs() into
arm_smmu_setup_irqs() and rename it to arm_smmu_resume_unique_irqs();
Call arm_smmu_setup_unique_irqs() to configure the IRQ during probe and
call arm_smmu_resume_unique_irqs() in resume mode to restore the IRQ
registers to make the code more reasonable.
* Call arm_smmu_device_disable() to disable smmu and clear CR0_SMMUEN on
suspend. Then the warning about CR0_SMMUEN being enabled can be cleared
on resume.
* Using SET_SYSTEM_SLEEP_PM_OPS();
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 69 ++++++++++++++++++---
1 file changed, 62 insertions(+), 7 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..66f35d5c7a70 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;
This is still broken.
MODULE_PARM_DESC(disable_msipolling,
"Disable MSI-based polling for CMD_SYNC completion.");
@@ -3129,11 +3130,38 @@ 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 == 0x0 && desc->msg.address_hi == 0x0) {
+ 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_unique_irqs(struct arm_smmu_device *smmu)
+{
+ struct msi_desc *desc;
+ struct msi_msg msg;
+
+ desc = irq_get_msi_desc(smmu->evtq.q.irq);
+ get_cached_msi_msg(smmu->evtq.q.irq, &msg);
+ arm_smmu_write_msi_msg(desc, &msg);
OK, now I think you actually *have* broken wired interrupts. What if
desc is NULL here?
+ desc = irq_get_msi_desc(smmu->gerr_irq);
+ get_cached_msi_msg(smmu->gerr_irq, &msg);
+ arm_smmu_write_msi_msg(desc, &msg);
+
+ if (smmu->features & ARM_SMMU_FEAT_PRI) {
+ desc = irq_get_msi_desc(smmu->priq.q.irq);
+ get_cached_msi_msg(smmu->priq.q.irq, &msg);
+ arm_smmu_write_msi_msg(desc, &msg);
+ }
+}
+
static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
{
struct msi_desc *desc;
@@ -3230,7 +3258,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;
@@ -3256,8 +3284,12 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
"arm-smmu-v3-combined-irq", smmu);
if (ret < 0)
dev_warn(smmu->dev, "failed to enable combined irq\n");
- } else
- arm_smmu_setup_unique_irqs(smmu);
+ } else {
+ if (!resume_mode)
+ arm_smmu_setup_unique_irqs(smmu);
+ else
+ arm_smmu_resume_unique_irqs(smmu);
+ }
This is still pretty much unreadable. As I said, a clear refactoring to
logically separate requesting the IRQs from programming the registers
would be the preferred way forward.
if (smmu->features & ARM_SMMU_FEAT_PRI)
irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
@@ -3282,7 +3314,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 +3424,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 +3781,25 @@ 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)
+{
+ struct arm_smmu_device *smmu = dev_get_drvdata(dev);
+
+ /* disable smmu to clear CR0 */
+ arm_smmu_device_disable(smmu);
Why? At worst this might actually cause more problems if there are any
client devices which have not been quiesced due to non-PM-aware drivers.
AFAICS this is just doing unnecessary work in the wrong place in an
attempt to avoid refactoring the reset routine. Don't do that, do it
properly.
It is the nature of kernel development that small changes build up over
time to a point where code starts to become unwieldy and deserves
refactoring. This code happens to be at that point - if making a small
logical change ends up needing a large invasive patch, that's usually a
sign that the code no longer has the best design for what it needs to
do, and *should* have the large invasive refactor for the sake of future
maintenance and development. Wanting to make the existing mess even more
convoluted because it's already enough of a mess that you're scared to
touch it too much is a self-defeating argument.
I ended up rewriting half of the SMMUv2 driver in the name of adding
suspend/resume support, precisely because doing so was also beneficial
in several other ways - what looks to want doing in this case is nowhere
near as involved as that was.
Robin.
+ 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,10 @@ 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 = {
+ SET_SYSTEM_SLEEP_PM_OPS(arm_smmu_suspend, arm_smmu_resume)
+};
+
static void arm_smmu_driver_unregister(struct platform_driver *drv)
{
arm_smmu_sva_notifier_synchronize();
@@ -3895,6 +3949,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,