Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Anna Maniscalco <anna.maniscalco2000@gmail.com>,
	Rob Clark <robin.clark@oss.qualcomm.com>,
	Will Deacon <will@kernel.org>, Joerg Roedel <joro@8bytes.org>
Cc: iommu@lists.linux.dev, linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] iommu: arm-smmu-qcom: Ensure smmu is powered up in set_ttbr0_cfg
Date: Thu, 7 May 2026 15:47:38 +0100	[thread overview]
Message-ID: <d071cc86-0449-430f-a572-4f43c3440748@arm.com> (raw)
In-Reply-To: <20260325-qcom_smmu_pmfix-v2-1-ba769a6ad0be@gmail.com>

On 25/03/2026 9:11 pm, Anna Maniscalco wrote:
> Previously the device was being accessed while potentially in a
> suspended state.

The SMMU driver's own write_context_bank() calls all look to be safely 
within arm_smmu_rpm_get() scopes that are holding an RPM count when 
relevant (or the one at probe time before RPM is activated at all), so 
indeed this seems like the appropriate solution for here where we're 
free to assume that Adreno platforms must always have a power domain.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Just a minor nit: it's probably OK in this case (unless Will disagrees), 
but for future patches, it's preferable for the commit message to be 
self-contained, so as to give a sufficiently clear description of the 
problem, and summary of the solution, without having to be read in the 
context of the title and the whole diff to make sense. For example, if I 
were writing this I'd put it something like:

"
   iommu/arm-smmu-qcom: Ensure SMMU is powered up in set_ttbr0_cfg

   arm_smmu_write_context_bank() assumes it is being called with RPM
   active, but it turns out that is not guaranteed in the path from
   qcom_adreno_smmu_set_ttbr0_cfg(), so it's possible for the the
   register accesses to lead to an [external abort/hang/whatever] when
   [doing whatever it is] while the GPU is idle. Add the RPM calls here
   to make sure the SMMU is active before we touch it.
"

Thanks,
Robin.

> Signed-off-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>
> ---
> Changes in v2:
> - Simplify patch by acquiring device just around the call that needs it
> - Link to v1: https://lore.kernel.org/r/20260210-qcom_smmu_pmfix-v1-1-78b7143ac053@gmail.com
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 573085349df3..cab7d110aaf5 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -231,6 +231,7 @@ static int qcom_adreno_smmu_set_ttbr0_cfg(const void *cookie,
>   	struct io_pgtable *pgtable = io_pgtable_ops_to_pgtable(smmu_domain->pgtbl_ops);
>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>   	struct arm_smmu_cb *cb = &smmu_domain->smmu->cbs[cfg->cbndx];
> +	int ret;
>   
>   	/* The domain must have split pagetables already enabled */
>   	if (cb->tcr[0] & ARM_SMMU_TCR_EPD1)
> @@ -260,8 +261,16 @@ static int qcom_adreno_smmu_set_ttbr0_cfg(const void *cookie,
>   		cb->ttbr[0] |= FIELD_PREP(ARM_SMMU_TTBRn_ASID, cb->cfg->asid);
>   	}
>   
> +	ret = pm_runtime_resume_and_get(smmu_domain->smmu->dev);
> +	if (ret < 0) {
> +		dev_err(smmu_domain->smmu->dev, "failed to get runtime PM: %d\n", ret);
> +		return -ENODEV;
> +	}
> +
>   	arm_smmu_write_context_bank(smmu_domain->smmu, cb->cfg->cbndx);
>   
> +	pm_runtime_put_autosuspend(smmu_domain->smmu->dev);
> +
>   	return 0;
>   }
>   
> 
> ---
> base-commit: 50c4a49f7292b33b454ea1a16c4f77d6965405dc
> change-id: 20260210-qcom_smmu_pmfix-2aead2ba4e20
> 
> Best regards,



  parent reply	other threads:[~2026-05-07 14:47 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-03-25 21:11 [PATCH v2] iommu: arm-smmu-qcom: Ensure smmu is powered up in set_ttbr0_cfg Anna Maniscalco
2026-05-07  8:21 ` Xilin Wu
2026-05-07 13:27   ` Anna Maniscalco
2026-05-07 14:12 ` Rob Clark
2026-05-07 14:47 ` Robin Murphy [this message]
2026-05-07 15:44   ` Anna Maniscalco

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d071cc86-0449-430f-a572-4f43c3440748@arm.com \
    --to=robin.murphy@arm.com \
    --cc=anna.maniscalco2000@gmail.com \
    --cc=iommu@lists.linux.dev \
    --cc=joro@8bytes.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robin.clark@oss.qualcomm.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox