linux-kernel - Re: [PATCH v2 1/2] drm/i915: Call dma_set_max_seg_size() in i915_ggtt_probe

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] [day] [month] [year] [list]

Message-ID: <156659001202.4019.17272565895689332680@skylake-alporthouse-com>
Date:   Fri, 23 Aug 2019 20:53:32 +0100
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Lyude Paul <lyude@...hat.com>, intel-gfx@...ts.freedesktop.org
Cc:     stable@...r.kernel.org, Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] drm/i915: Call dma_set_max_seg_size() in
 i915_ggtt_probe_hw()

Quoting Lyude Paul (2019-08-22 21:31:26)
> Currently, we don't call dma_set_max_seg_size() for i915 because we
> intentionally do not limit the segment length that the device supports.
> However, this results in a warning being emitted if we try to map
> anything larger than SZ_64K on a kernel with CONFIG_DMA_API_DEBUG_SG
> enabled:
> 
> [    7.751926] DMA-API: i915 0000:00:02.0: mapping sg segment longer
> than device claims to support [len=98304] [max=65536]
> [    7.751934] WARNING: CPU: 5 PID: 474 at kernel/dma/debug.c:1220
> debug_dma_map_sg+0x20f/0x340
> 
> This was originally brought up on
> https://bugs.freedesktop.org/show_bug.cgi?id=108517 , and the consensus
> there was it wasn't really useful to set a limit (and that dma-debug
> isn't really all that useful for i915 in the first place). Unfortunately
> though, CONFIG_DMA_API_DEBUG_SG is enabled in the debug configs for
> various distro kernels. Since a WARN_ON() will disable automatic problem
> reporting (and cause any CI with said option enabled to start
> complaining), we really should just fix the problem.
> 
> Note that as me and Chris Wilson discussed, the other solution for this
> would be to make DMA-API not make such assumptions when a driver hasn't
> explicitly set a maximum segment size. But, taking a look at the commit
> which originally introduced this behavior, commit 78c47830a5cb
> ("dma-debug: check scatterlist segments"), there is an explicit mention
> of this assumption and how it applies to devices with no segment size:
> 
>         Conversely, devices which are less limited than the rather
>         conservative defaults, or indeed have no limitations at all
>         (e.g. GPUs with their own internal MMU), should be encouraged to
>         set appropriate dma_parms, as they may get more efficient DMA
>         mapping performance out of it.
> 
> So unless there's any concerns (I'm open to discussion!), let's just
> follow suite and call dma_set_max_seg_size() with UINT_MAX as our limit
> to silence any warnings.
> 
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> Cc: Chris Wilson <chris@...is-wilson.co.uk>
> Cc: <stable@...r.kernel.org> # v4.18+
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0b81e0b64393..a1475039d182 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3152,6 +3152,11 @@ static int ggtt_probe_hw(struct i915_ggtt *ggtt, struct intel_gt *gt)
>         if (ret)
>                 return ret;
>  
> +       /* We don't have a max segment size, so set it to the max so sg's
> +        * debugging layer doesn't complain
> +        */
> +       dma_set_max_seg_size(ggtt->vm.dma, UINT_MAX);

The rest of the dma setup is in i915_driver_hw_probe, so I would put it
there just after dma_set_coherent_mask() (maybe one day even being brave
enough to pull out those to their own function).

I think I've made my point about the futility of dma-debug and you've
made yours about the simplicity of the patch to shut it up, so move it
across and
Reviewed-by: Chris Wilson <chris@...is-wilson.co.uk>
-Chris