Re: [PATCH -next v2] iommu/arm-smmu-v3: Add suspend and resume support

[Marc Zyngier <maz@kernel.org>]

On Tue, 27 Jul 2021 13:14:08 +0100,
Bixuan Cui <cuibixuan@huawei.com> 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>
> ---
> 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;

As outlined before, this is likely to be wrong if you can have
per-SMMU bypass control.

>  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;
> +	}

I thought I had made it clear that this approach is not acceptable.
Please fix the generic code to keep track of the latest message.

Thanks,

	M.

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