Message-ID: <f4371199-63ad-0487-3066-8149d007b416@amd.com>
Date:   Mon, 20 Mar 2023 16:25:21 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH] drm/amd/display: use a more accurate check in
 dm_helpers_dp_read_dpcd()
Content-Language: en-US
To:     =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        Hamza Mahfooz <hamza.mahfooz@amd.com>
Cc:     Ian Chen <ian.chen@amd.com>, Leo Li <sunpeng.li@amd.com>,
        Qingqing Zhuo <qingqing.zhuo@amd.com>,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>,
        linux-kernel@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, Wayne Lin <Wayne.Lin@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Sung Joon Kim <sungjoon.kim@amd.com>
References: <20230309213027.256243-1-hamza.mahfooz@amd.com>
 <ZAttVGN/VdrfkH3y@intel.com> <ZAtuET8c8kSgyX4Y@intel.com>
From:   Harry Wentland <harry.wentland@amd.com>
In-Reply-To: <ZAtuET8c8kSgyX4Y@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?blo5TUs0M3UvL2kxUEQyRG5WaFFVdHlGUld0ekkrM1hGeTc2WkErKzBndnc4?=
 =?utf-8?B?WS9DNWpPVDFBcFNYNVFnOWJQTDNOamFidGNjOFVSSDJvZXU0L1RiTFhDaXcv?=
 =?utf-8?B?YXcrZ1dSdXVIalhNcndjOHEzNEdSeTlWWmtCUHBnR3ZHQ1h3Wmo0VFh6N3VH?=
 =?utf-8?B?MlNpV0NWL1dDZWgwd2h5UUtqUmpCZThKYi9iTEd1eWFSRThLcEtoYzhvRUdR?=
 =?utf-8?B?SHNPaUZHOE9NeXlYSkYyNkRmb25RcnBrZTF1VDhaVXRTdUx3R1d0eFJYallU?=
 =?utf-8?B?Y0M2TWkweDVNbDh3SXVtSGVyOTQ5VkN6LzJnaFRmbHcwMHlUcUljMEZmSk8w?=
 =?utf-8?B?bHRWYnVzaG03NEpjRFlrbnIwd3dkcW5RdFpQTEFIeG5lTWZoVFgyZlJ4Ykw2?=
 =?utf-8?B?V3RTWDMxamgvUDNXUGtDSGdtUEVJd0NxcmkxWG9ZQVBlWG8yOTNQMGFZdFRD?=
 =?utf-8?B?OXdFNG1lNE4yb0phT2MzRmRiODFyMnhTc0FlRCtaWnZ6bGl1QXhhL045di93?=
 =?utf-8?B?b2xPMlZLN0g2TUl4disxdDZZU3p3ZHZmKzlFSWY5UGhXZE5uWjRlSFF2YnJR?=
 =?utf-8?B?dmVBbmJXVEYrNGVicEhOdkppa1IzQ05BVU40aGtSSzBKQzQycDVaTTF6Nmlu?=
 =?utf-8?B?MzE4ZTJLR2p0OVBwNUZnMkJZMG5xOTVVdW9PTGh0ODFWN3B1QndVRFFZK3po?=
 =?utf-8?B?UEdTRTZTR1Y0b20wUzdEbnUvUmtzbzFyL1JxRy9yTy9iUVJ0YUhXZFNZS1N0?=
 =?utf-8?B?cVRvZDR0bGVyaVdZMnVGK1lob1JFMlp6MThjZEdZY0RObjl0bkVxK3JvZmJU?=
 =?utf-8?B?THVJa091cSs2K2ZpaHM5a2JmSXNJNnRPZDZzTlJWdkZoa2FQdDZ4WmlJK015?=
 =?utf-8?B?UUt0RWZuOFR1Tks0OTA0Y3Bma1ZkZFBBczhKYkFKSEM5UXFEalNra1hmT1k3?=
 =?utf-8?B?S0RNVEQ3V2IrRUhDR0RuRDh3bURORk5SeDFhNGg4a2g3bllmU0JISHcrUnZB?=
 =?utf-8?B?RVpydnZoN0MvY2sxcythdGNISS81Z1FlT20wTFU3VWc4anl2SWMzbHBNeVBU?=
 =?utf-8?B?VGgxM0N0cG5IOGhQZmFKSzNvTHpweXh4T0xsR0Z1UmRSZHZrVlp4K1BJZFMr?=
 =?utf-8?B?Nktia0laKzRYMUIrVG11YU9YUGVYb1dRWVEzYmRJK0JBcjR4TEphNjUxb0VD?=
 =?utf-8?B?S0pZeENYT2F1Q1IxS0tIdlhpdzZKODVXUTdSU25SaXRvekRRbTVzckR5TDFB?=
 =?utf-8?B?T0FYNUpIMUV6R2J5ZFMvd3RGOXdYejFXV3Q4ajlWQlJ3K1JDaFNqblB2MzhP?=
 =?utf-8?B?NXZRN0dHMlRBZnVxQVFlSS9SR25rcnRpVjZBa2ltM0RhNlROWVRENkhWRFZr?=
 =?utf-8?B?V1VzS0pjV2hnVVhKNzJwa0M0VTg4bklHZzdiRUN0VTl6U0J6ZFZUdWhvdFRS?=
 =?utf-8?B?N25lQTF1bko3S3JLOUttOUJuVjVqYklLOE1EYzlEeHI2b1ZrVEY5VGVHSEJo?=
 =?utf-8?B?d05DaE5YY2p6R1ZhY210WEIvdkxtM0lYNEpMZGM1dGp3elBOaGtaZVRRVDRL?=
 =?utf-8?B?TDF0dEpMN0s4MzU4M0RrcGQyWml5MGc3TVNpSDRVZW9iWk03OWd3U0JnZXNs?=
 =?utf-8?B?UEZtQ21IbFdmZGtneG5ETlNLQ0dQbW5WQUsvSFYxaWJHNWZWNElWSEpOemh6?=
 =?utf-8?B?eExHbE1TYnJvZktrZUtEVEM2Vm5ITUR2Z0pKcElMT3RBRnR1MGFBKzRDQjVR?=
 =?utf-8?B?blF1RG5uYmRuekdpSU1DemxQRzJ0eDVzTHpkVWdhZm0yVDhpVTR3R2ZZVG9s?=
 =?utf-8?B?N1JRRkpFZlNUM012UjB4QXhDbkwzUjFaVkhmbjhMRE1Wd3dDVVZLSkNrcEN0?=
 =?utf-8?B?dUd3NGJ5NjVSZWdMTm5Yb09mNzRhYVJjeldoKzFpemdRdDEvN2NFQkpTS0RS?=
 =?utf-8?B?OHFDaFJvOTBnckFlVVZyeGhNdXRzK1g3aW1HZFV5b2tseHM5VFhvNFBYYWpn?=
 =?utf-8?B?TjVRQTBqNW02WGpsK21rcSt2SlFud1BmQ1EzOHpWTzB3djRHSXYwbGR5eTNR?=
 =?utf-8?B?cVVLcTRGajZNQ0lqOVVyTzBnbXE0blY1YUx1Zm55blRWNGJzT2lGaEV2QnFD?=
 =?utf-8?Q?GyAul+c3IcNDPH/GanBp5UWNi?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d13829d6-e090-4291-537b-08db29813d36
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5427.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2023 20:25:26.0047
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: lzdkMajI59HVyeRzUClyw+hHO/wgs8Yg3P4ppkPy4gbtXrX6cNYwraJ3V0RUgEUT31fi5bOqle2gHdM7xiunwQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5065
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 3/10/23 12:51, Ville Syrjälä wrote:
> On Fri, Mar 10, 2023 at 07:48:04PM +0200, Ville Syrjälä wrote:
>> On Thu, Mar 09, 2023 at 04:30:27PM -0500, Hamza Mahfooz wrote:
>>> We should be checking if drm_dp_dpcd_read() returns the size that we are
>>> asking it to read instead of just checking if it is greater than zero.
>>> Also, we should WARN_ON() here since this condition is only ever met, if
>>> there is an issue worth investigating. So, compare the return value of
>>> drm_dp_dpcd_read() to size and WARN_ON() if they aren't equal.
>>>
>>> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> index 8d598b322e5b..ed2ed7b1d869 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>>> @@ -511,8 +511,8 @@ bool dm_helpers_dp_read_dpcd(
>>>  		return false;
>>>  	}
>>>  
>>> -	return drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
>>> -			data, size) > 0;
>>> +	return !WARN_ON(drm_dp_dpcd_read(&aconnector->dm_dp_aux.aux, address,
>>> +					 data, size) != size);
>>
>> Just FYI there are devices out there that violate the DP spec and reads
>> from specific DPCD registers simply fail instead of returning the
>> expected 0.
> 
> And of course anyone can yank the cable anytime, so in
> fact pretty much any DPCD read can fail.
> 

Thanks for making this very important point. It seems like drm_dp_dpcd_access
checks for that, though, and returns -EPROTO if !(ret == size). So I don't
expect this patch to change any behavior.

Harry