Re: [PATCH v11 13/16] iommu/arm-smmu-v3: Implement pm_runtime & system sleep ops

[Nicolin Chen <nicolinc@nvidia.com>]

Just some small things from a quick scan:

On Tue, Sep 29, 2026 at 03:45:07AM +0000, Pranjal Shrivastava wrote:
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 0a841441cc44..d0a3d497915c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -663,11 +663,14 @@ arm_smmu_make_cmd_tlbi(enum arm_smmu_cmdq_opcode op, u16 asid, u16 vmid)
[...]
> +#define RPM_AUTOSUSPEND_DELAY_MS	15
> +
>  struct arm_smmu_ll_queue {
>  	union {
>  		u64			val;
> @@ -1234,6 +1237,18 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
>  				bool sync);
>  bool arm_smmu_erratum_repeat_tlbi_cfgi(void);
>  
> +/*
> + * Lockless pre-check to test if the SMMU is actively powered.
> + * Races with concurrent suspend are benign: the cmpxchg loop in
> + * arm_smmu_cmdq_issue_cmdlist() acts as the true commit point.
> + * If we lose the race, that loop observes Q_STOP == 1 and safely
> + * drops the command. If we win, the suspend thread waits for us.
> + */
> +static inline bool arm_smmu_is_active(struct arm_smmu_device *smmu)
> +{
> +	return !Q_STOP(READ_ONCE(smmu->cmdq.q.llq.prod));
> +}

Move these to the patch that uses it? 

> 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 89a4e756ff17..e127e3be3a6b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -29,6 +29,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/sort.h>
>  #include <linux/string_choices.h>
> +#include <linux/pm_runtime.h>
>  #include <kunit/visibility.h>
>  #include <uapi/linux/iommufd.h>
>  
> @@ -119,6 +120,45 @@ static const char * const event_class_str[] = {
>  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
>  static bool arm_smmu_ats_supported(struct arm_smmu_master *master);
>  
> +/* Runtime PM helpers */
> +__maybe_unused static int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> +{
> +	int ret;
> +
> +	if (!pm_runtime_enabled(smmu->dev))
> +		return 0;
> +
> +	ret = pm_runtime_resume_and_get(smmu->dev);
> +	if (ret < 0) {
> +		dev_err(smmu->dev, "failed to resume device: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +__maybe_unused static bool arm_smmu_rpm_get_if_active(struct arm_smmu_device *smmu)
> +{
> +	if (!pm_runtime_enabled(smmu->dev))
> +		return true;
> +
> +	return pm_runtime_get_if_active(smmu->dev) > 0;
> +}
> +
> +__maybe_unused static void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> +{
> +	int ret;
> +
> +	if (!pm_runtime_enabled(smmu->dev))
> +		return;
> +
> +	ret = pm_runtime_put_autosuspend(smmu->dev);
> +
> +	/* -EAGAIN & -EBUSY aren't failures */
> +	if (ret < 0 && ret != -EAGAIN && ret != -EBUSY)
> +		dev_err(smmu->dev, "failed to suspend device: %d\n", ret);
> +}

Ditto

> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
[...]
> +	/* Wait for the last committed owner to reach the hardware */
> +	while (atomic_read(&cmdq->owner_prod) != target && timeout) {
> +		udelay(1);
> +		timeout--;
> +	}
> +
> +	/*
> +	 * Entering suspend implies no active clients. A timeout here
> +	 * indicates a fatal CMDQ lockup or hardware stall. We proceed
> +	 * anyway to prioritize memory safety (avoiding stale TLBs)
> +	 */
> +	if (!timeout)
> +		dev_err(smmu->dev, "cmdq owner wait timeout, (check runtime PM + devlinks)\n");
> +
> +	/* Wait for cmdq->lock == 0 to ensure last CMDQ_CONS_REG is written */
> +	timeout = ARM_SMMU_SUSPEND_TIMEOUT_US;
> +	while (atomic_read(&cmdq->lock) != 0 && timeout) {
> +		udelay(1);
> +		timeout--;
> +	}
> +
> +	/* Timing out here implies misconfigured Runtime PM or broken devlinks */
> +	if (!timeout)
> +		dev_err(smmu->dev, "cmdq lock != 0, forcing suspend. Polling CPUs may fault.\n");

Maybe a small helper that takes: read pointer, target, print?

> +	/* Wait for pending gerror handlers */
> +	synchronize_irq(smmu->combined_irq ? smmu->combined_irq : smmu->gerr_irq);

Both might be 0?

Nicolin