Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1759383AbcCVOca (ORCPT <rfc822;w@1wt.eu>);
	Tue, 22 Mar 2016 10:32:30 -0400
Received: from mail-yw0-f180.google.com ([209.85.161.180]:33730 "EHLO
	mail-yw0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754003AbcCVOcV (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 22 Mar 2016 10:32:21 -0400
MIME-Version: 1.0
In-Reply-To: <87vb4ewv9m.fsf@gmail.com>
References: <1458562256-2882-1-git-send-email-nicstange@gmail.com>
	<CADnq5_N8brxsQMMiU_sz38i_WC5g=R3PLMv-tt3P8kdQUZgk7Q@mail.gmail.com>
	<87vb4ewv9m.fsf@gmail.com>
Date: Tue, 22 Mar 2016 10:32:20 -0400
Message-ID: <CADnq5_NBy0s1xs=MWBThJiF1BrTwdGAxMZ9zeJE7HXpycOPJGg@mail.gmail.com>
Subject: Re: [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
From: Alex Deucher <alexdeucher@gmail.com>
To: Nicolai Stange <nicstange@gmail.com>
Cc: David Airlie <airlied@linux.ie>, Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Maling list - DRI developers 
	<dri-devel@lists.freedesktop.org>
Content-Type: text/plain; charset=UTF-8
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 5191
Lines: 122

On Tue, Mar 22, 2016 at 8:29 AM, Nicolai Stange <nicstange@gmail.com> wrote:
> Alex Deucher <alexdeucher@gmail.com> writes:
>
>> On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> 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:
>>>    [<ffffffff818c4d35>] dump_stack+0xbc/0x117
>>>    [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
>>>    [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
>>>    [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
>>>    [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
>>>    [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
>>>    [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
>>>    [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
>>>    [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
>>>    [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
>>>    [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
>>>    [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
>>>    [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
>>>    [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
>>>    [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
>>>    [<ffffffff814b83f0>] ? kfree+0x220/0x370
>>>    [<ffffffffa0b687c2>] 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 <nicstange@gmail.com>
>>
>> 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?

Yes, that would be fine.

Alex

>
>
> 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