| Message ID | CAPM=9tyKdGHyiRLDooKrMf=02GtNn8U4YfF4dJtXdabnVAGdXQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [git,pull] drm fixes for 5.12 final | expand |
The pull request you sent on Fri, 23 Apr 2021 13:52:29 +1000:
> git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2021-04-23
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5bfc75d92efd494db37f5c4c173d3639d4772966
Thank you!
I've updated to Fedora 34 on one of my machines, and it causes a lot
of i915 warnings like
drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
of type ‘const u16 *’ {aka ‘const short unsigned int *’}
drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
‘intel_print_wm_latency’
and the reason is that gcc now seems to look at the argument array
size more, and notices that
(a) intel_print_wm_latency() takes a "const u16 wm[8]" argument
but
(b) most of the arrays passed in tend to look like 'u16 pri_latency[5]'
I think I will make the argument type to intel_print_wm_latency() be
just "const u16 wm[]" for now, just to avoid seeing a ton of silly
warnings.
I'm not sure if there is a better solution (like making all of those
latency arrays be 8 entries in size), so I'm just letting you know
about my change in this area in case anybody has a better idea.
Linus
On Tue, Apr 27, 2021 at 4:43 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > I think I will make the argument type to intel_print_wm_latency() be > just "const u16 wm[]" for now, just to avoid seeing a ton of silly > warnings. After fixing the trivial ones, this one remains: drivers/gpu/drm/i915/display/intel_dp.c: In function ‘intel_dp_check_mst_status’: drivers/gpu/drm/i915/display/intel_dp.c:4554:22: warning: ‘drm_dp_channel_eq_ok’ reading 6 bytes from a region of size 4 [-Wstringop-overread] 4554 | !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/display/intel_dp.c:4554:22: note: referencing argument 1 of type ‘const u8 *’ {aka ‘const unsigned char *’} In file included from drivers/gpu/drm/i915/display/intel_dp.c:38: ./include/drm/drm_dp_helper.h:1459:6: note: in a call to function ‘drm_dp_channel_eq_ok’ 1459 | bool drm_dp_channel_eq_ok(const u8 link_status[DP_LINK_STATUS_SIZE], | ^~~~~~~~~~~~~~~~~~~~ and I'm not fixing that one, because it actually looks like a valid warning, and doesn't have an obvious fix. That "esi[]" array is 14 bytes in size (DP_DPRX_ESI_LEN). So when it does that "&esi[10]" and passes it in as an argument, then only 4 bytes remain of the array. And drm_dp_channel_eq_ok() supposedly takes a "const u8 link_status[DP_LINK_STATUS_SIZE]", which is 6 bytes. There may be some reason this is ok, but it does look a bit fishy, and the compiler warning is appropriate. Linus
On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote: > I've updated to Fedora 34 on one of my machines, and it causes a lot > of i915 warnings like > > drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’: > drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3 > of type ‘const u16 *’ {aka ‘const short unsigned int *’} > drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function > ‘intel_print_wm_latency’ > > and the reason is that gcc now seems to look at the argument array > size more, and notices that Arnd Bergmann reported some of these a while back. I think we have some of them fixed in our -next already, but not all. Thanks for the reminder. BR, Jani.
I have heard nothing about this, and it remains the only warning from
my allmodconfig build (I have another one for drm compiled with clang,
but there I at least heard back that a fix exists).
Since I am going to release rc1 tomorrow, and I don't want to release
it with an ugly compiler warning, I took it upon myself to just fix
the code:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852
HOWEVER.
That commit fixes the warning, and is at worst harmless. At best it
fixes an access to random stack memory. But it does smell like
somebody who actually knows how these arrays work should look at that
code.
IOW, maybe the code should actually have read 16 bytes from the Event
Status Indicator? Maybe offset 10 was wrong? Maybe
drm_dp_channel_eq_ok() should never have taken six bytes to begin
with?
It's a mystery, and I haven't heard anything otherwise, so there it is.
Linus
On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@intel.com> wrote:
>
> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > I've updated to Fedora 34 on one of my machines, and it causes a lot
> > of i915 warnings like
> >
> > drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> > drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3
> > of type ‘const u16 *’ {aka ‘const short unsigned int *’}
> > drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> > ‘intel_print_wm_latency’
> >
> > and the reason is that gcc now seems to look at the argument array
> > size more, and notices that
>
> Arnd Bergmann reported some of these a while back. I think we have some
> of them fixed in our -next already, but not all. Thanks for the
> reminder.
On Sat, 08 May 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote: > I have heard nothing about this, and it remains the only warning from > my allmodconfig build (I have another one for drm compiled with clang, > but there I at least heard back that a fix exists). > > Since I am going to release rc1 tomorrow, and I don't want to release > it with an ugly compiler warning, I took it upon myself to just fix > the code: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fec4d42724a1bf3dcba52307e55375fdb967b852 > > HOWEVER. > > That commit fixes the warning, and is at worst harmless. At best it > fixes an access to random stack memory. But it does smell like > somebody who actually knows how these arrays work should look at that > code. > > IOW, maybe the code should actually have read 16 bytes from the Event > Status Indicator? Maybe offset 10 was wrong? Maybe > drm_dp_channel_eq_ok() should never have taken six bytes to begin > with? > > It's a mystery, and I haven't heard anything otherwise, so there it is. Fair enough. My bad for not getting this fixed. The fix is harmless. drm_dp_channel_eq_ok() only ever accesses 3 bytes instead of 6. I figure the DP_LINK_STATUS_SIZE (=6) is there because in the normal case you'd read that much, and use a family of functions on that data, some of which do access the full 6 bytes, some don't. In our case, we use drm_dp_channel_eq_ok() to check 3 bytes of similarly encoded data elsewhere in the DPCD address space, and the DP_LINK_STATUS_SIZE is meaningless there. The straightforward fix would be to replace link_status[DP_LINK_STATUS_SIZE] with link_status[3], and that likely needs changes in dp_link_status() and dp_get_lane_status() as well. BR, Jani. > > Linus > > On Wed, Apr 28, 2021 at 12:27 AM Jani Nikula <jani.nikula@intel.com> wrote: >> >> On Tue, 27 Apr 2021, Linus Torvalds <torvalds@linux-foundation.org> wrote: >> > I've updated to Fedora 34 on one of my machines, and it causes a lot >> > of i915 warnings like >> > >> > drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’: >> > drivers/gpu/drm/i915/intel_pm.c:3059:9: note: referencing argument 3 >> > of type ‘const u16 *’ {aka ‘const short unsigned int *’} >> > drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function >> > ‘intel_print_wm_latency’ >> > >> > and the reason is that gcc now seems to look at the argument array >> > size more, and notices that >> >> Arnd Bergmann reported some of these a while back. I think we have some >> of them fixed in our -next already, but not all. Thanks for the >> reminder.
Hi Linus, Just some small i915 and amdgpu fixes this week, Should be all until you open the merge window. Regards, Dave. drm-fixes-2021-04-23: drm fixes for 5.12 final amdgpu: - Fix gpuvm page table update issue - Modifier fixes - Register fix for dimgrey cavefish i915: - GVT's BDW regression fix for cmd parser - Fix modesetting in case of unexpected AUX timeouts The following changes since commit bf05bf16c76bb44ab5156223e1e58e26dfe30a88: Linux 5.12-rc8 (2021-04-18 14:45:32 -0700) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2021-04-23 for you to fetch changes up to aca38735ae624b93c71c055b68d5802b8f356ea5: Merge tag 'drm-intel-fixes-2021-04-22' of git://anongit.freedesktop.org/drm/drm-intel into drm-fixes (2021-04-23 12:18:21 +1000) ---------------------------------------------------------------- drm fixes for 5.12 final amdgpu: - Fix gpuvm page table update issue - Modifier fixes - Register fix for dimgrey cavefish i915: - GVT's BDW regression fix for cmd parser - Fix modesetting in case of unexpected AUX timeouts ---------------------------------------------------------------- Dave Airlie (2): Merge tag 'amd-drm-fixes-5.12-2021-04-21' of https://gitlab.freedesktop.org/agd5f/linux into drm-fixes Merge tag 'drm-intel-fixes-2021-04-22' of git://anongit.freedesktop.org/drm/drm-intel into drm-fixes Imre Deak (1): drm/i915: Fix modesetting in case of unexpected AUX timeouts Jiansong Chen (1): drm/amdgpu: fix GCR_GENERAL_CNTL offset for dimgrey_cavefish Philip Yang (1): drm/amdgpu: reserve fence slot to update page table Qingqing Zhuo (1): drm/amd/display: Update modifier list for gfx10_3 Rodrigo Vivi (1): Merge tag 'gvt-fixes-2021-04-20' of https://github.com/intel/gvt-linux into drm-intel-fixes Simon Ser (1): amd/display: allow non-linear multi-planar formats Zhenyu Wang (1): drm/i915/gvt: Fix BDW command parser regression drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 ++++++++-- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 2 +- drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 ++++++--------- drivers/gpu/drm/i915/display/intel_dp_link_training.c | 3 ++- drivers/gpu/drm/i915/gvt/cmd_parser.c | 19 +++++++++++++------ 5 files changed, 30 insertions(+), 19 deletions(-)