linux-kernel - Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Feb 2018 18:10:38 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Vivek Gautam <vivek.gautam@...eaurora.org>
Cc:     "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>," <joro@...tes.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Robin Murphy <robin.murphy@....com>,
        Will Deacon <will.deacon@....com>,
        Rob Clark <robdclark@...il.com>,
        "list@....net:IOMMU DRIVERS <iommu@...ts.linux-foundation.org>, Joerg
        Roedel <joro@...tes.org>," <iommu@...ts.linux-foundation.org>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        freedreno@...ts.freedesktop.org, David Airlie <airlied@...ux.ie>,
        Greg KH <gregkh@...uxfoundation.org>, sboyd@...eaurora.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
<vivek.gautam@...eaurora.org> wrote:
> While handling the concerned iommu, there should not be a
> need to power control the drm devices from iommu interface.
> If these drm devices need to be powered around this time,
> the respective drivers should take care of this.
>
> Replace the pm_runtime_get/put_sync(<drm_device>) with
> pm_runtime_get/put_suppliers(<drm_device>) calls, to power-up
> the connected iommu through the device link interface.
> In case the device link is not setup these get/put_suppliers()
> calls will be a no-op, and the iommu driver should take care of
> powering on its devices accordingly.
>
> Signed-off-by: Vivek Gautam <vivek.gautam@...eaurora.org>
> ---
>  drivers/gpu/drm/msm/msm_iommu.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index b23d33622f37..1ab629bbee69 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
> @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names,
>         struct msm_iommu *iommu = to_msm_iommu(mmu);
>         int ret;
>
> -       pm_runtime_get_sync(mmu->dev);
> +       pm_runtime_get_suppliers(mmu->dev);
>         ret = iommu_attach_device(iommu->domain, mmu->dev);
> -       pm_runtime_put_sync(mmu->dev);
> +       pm_runtime_put_suppliers(mmu->dev);

For me, it looks like a wrong place to handle runtime PM of IOMMU
here. iommu_attach_device() calls into IOMMU driver's attach_device()
callback and that's where necessary runtime PM gets should happen, if
any. In other words, driver A (MSM DRM driver) shouldn't be dealing
with power state of device controlled by driver B (ARM SMMU).

This is also important for the reasons I stated in my comments to
"[PATCH v7 1/6] base: power: runtime: Export
pm_runtime_get/put_suppliers". Quoting for everyone's convenience:

>> There are however cases in which the consumer wants to power-on
>> the supplier, but not itself.
>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>> to unmap a buffer and finish the TLB operations without powering
>> on itself.
>
>This sounds strange to me. If the SMMU is powered down, wouldn't the
>TLB lose its contents as well (and so no flushing needed)?
>
>Other than that, what kind of hardware operations would be needed
>besides just updating the page tables from the CPU?
>

In other words, the SMMU driver can deal with hardware state based on
return value of pm_runtime_get_sync() or pm_runtime_get_if_in_use()
and decide whether some operations are necessary or not, e.g.
- a state restore is necessary if the domain was powered off, but we
are bringing the master on,
- a flush may not be required when (un)mapping with the domain powered off,
- etc.

Best regards,
Tomasz