Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758502AbcCVM3v (ORCPT ); Tue, 22 Mar 2016 08:29:51 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35285 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756528AbcCVM3o (ORCPT ); Tue, 22 Mar 2016 08:29:44 -0400 From: Nicolai Stange To: Alex Deucher Cc: Nicolai Stange , David Airlie , Alex Deucher , Christian =?utf-8?Q?K=C3=B6ni?= =?utf-8?Q?g?= , LKML , Maling list - DRI developers Subject: Re: [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset References: <1458562256-2882-1-git-send-email-nicstange@gmail.com> Date: Tue, 22 Mar 2016 13:29:41 +0100 In-Reply-To: (Alex Deucher's message of "Mon, 21 Mar 2016 12:09:24 -0400") Message-ID: <87vb4ewv9m.fsf@gmail.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4950 Lines: 116 Alex Deucher writes: > On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange wrote: >> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id >> enum transform 1:1 into bit positions within the 'enabled' bitset as >> assembled by evergreen_hpd_init(): >> >> enabled |= 1 << radeon_connector->hpd.hpd; >> >> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN >> reports >> >> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16 >> shift exponent 255 is too large for 32-bit type 'int' >> [...] >> Call Trace: >> [] dump_stack+0xbc/0x117 >> [] ? _atomic_dec_and_lock+0x169/0x169 >> [] ubsan_epilogue+0xd/0x4e >> [] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254 >> [] ? atom_execute_table+0x3e/0x50 [radeon] >> [] ? __ubsan_handle_load_invalid_value+0x158/0x158 >> [] ? radeon_get_pll_use_mask+0x130/0x130 [radeon] >> [] ? wake_up_klogd_work_func+0x60/0x60 >> [] ? vprintk_default+0x3e/0x60 >> [] evergreen_hpd_init+0x274/0x2d0 [radeon] >> [] ? evergreen_hpd_init+0x274/0x2d0 [radeon] >> [] radeon_modeset_init+0x8ce/0x18d0 [radeon] >> [] radeon_driver_load_kms+0x186/0x350 [radeon] >> [] drm_dev_register+0xc6/0x100 [drm] >> [] drm_get_pci_dev+0xe4/0x490 [drm] >> [] ? kfree+0x220/0x370 >> [] radeon_pci_probe+0x112/0x140 [radeon] >> [...] >> ===================================================================== >> radeon 0000:01:00.0: No connectors reported connected with modes >> >> At least on x86, there should be no user-visible impact as there >> >> 1 << 0xff == 1 << (0xff & 31) == 1 << 31 >> >> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one. >> >> All of the above applies analogously to evergreen_hpd_fini(). >> >> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it >> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset >> in evergreen_hpd_fini() respectively. >> >> Signed-off-by: Nicolai Stange > > Can you please fix all instances of this? This same pattern is > repeated in various asic families. Sure. The places to fix are: $ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/ drivers/gpu/drm/radeon/evergreen.c:1868: enabled |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/evergreen.c:1912: disabled |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r100.c:595: enable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r100.c:617: disable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r600.c:1005: enable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/r600.c:1058: disable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/rs600.c:416: enable |= 1 << radeon_connector->hpd.hpd; drivers/gpu/drm/radeon/rs600.c:442: disable |= 1 << radeon_connector->hpd.hpd; May I throw all of those into one single patch? Thanks, Nicolai > >> --- >> Applicable to linux-next-20160321. >> >> Changes to v1: >> - Turn commit message's impact part into a non-impact part. >> >> drivers/gpu/drm/radeon/evergreen.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c >> index 76c4bdf..6360717 100644 >> --- a/drivers/gpu/drm/radeon/evergreen.c >> +++ b/drivers/gpu/drm/radeon/evergreen.c >> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev) >> break; >> } >> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd); >> - enabled |= 1 << radeon_connector->hpd.hpd; >> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) >> + enabled |= 1 << radeon_connector->hpd.hpd; >> } >> radeon_irq_kms_enable_hpd(rdev, enabled); >> } >> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev) >> default: >> break; >> } >> - disabled |= 1 << radeon_connector->hpd.hpd; >> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE) >> + disabled |= 1 << radeon_connector->hpd.hpd; >> } >> radeon_irq_kms_disable_hpd(rdev, disabled); >> } >> -- >> 2.7.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel