<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72"> <div class="WordSection1"> [AMD Public Use]<o:p></o:p> <o:p> </o:p> Responses inline<o:p></o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Chen, Guchun <Guchun.Chen@amd.com> Sent: Friday, May 8, 2020 3:11 PM To: Clements, John <John.Clements@amd.com>; amd-gfx@lists.freedesktop.org; Zhang, Hawking <Hawking.Zhang@amd.com> Subject: RE: [PATCH] Series to disallow XGMI link power down during RAS XGMI error injection <o:p></o:p> </div> </div> <o:p> </o:p> [AMD Public Use]<o:p></o:p> <o:p> </o:p> Patch 1, <o:p></o:p> 1. maybe it’s better to split it into 2, one is for header file change, and another is functional patch.<o:p></o:p> 2. Can below codes be merged to one return instead? Variable ‘en’ can serve the function input in smu_send_smc_msg_with_param.<o:p></o:p> [JC] I think what you are suggesting would probably work, but my concern is that it is not immediate clear to me if bool ec will compile to 1. The spec for the SMU cmd defines 1 to enable and 0 to disable<o:p></o:p> if (en)<o:p></o:p> + return smu_send_smc_msg_with_param(smu,<o:p></o:p> + SMU_MSG_GmiPwrDnControl,<o:p></o:p> + 1,<o:p></o:p> + NULL);<o:p></o:p> +<o:p></o:p> + return smu_send_smc_msg_with_param(smu,<o:p></o:p> + SMU_MSG_GmiPwrDnControl,<o:p></o:p> + 0,<o:p></o:p> + NULL);<o:p></o:p> <o:p> </o:p> <div> Patch 2: Reviewed-by: Guchun Chen <a href="mailto:guchun.chen@amd.com"> guchun.chen@amd.com</a><o:p></o:p> <o:p> </o:p> Patch 3:<o:p></o:p> 1.function amdgpu_ras_error_inject_xgmi should be one static function? [JC] That makes sense, I’ll update function to be static<o:p></o:p> 2. Regarding below return case, is there one dpm_allow_xgmi_power_down leak? As we have disabled xgmi link power down.<o:p></o:p> [JC] I intentionally did this, when the RAS int is triggered I don’t want to create any races conditions to access the SMU to perform functions and entering BACO reset, the XGMI per link settings will be restored anyway after the BACO reset.<o:p></o:p> [JC] That being said I understand that this looks like it could be a bug, I’ll add comments there to clarify the sequence<o:p></o:p> + if (amdgpu_ras_intr_triggered())<o:p></o:p> + return ret;<o:p></o:p> <o:p> </o:p> Regards,<o:p></o:p> Guchun<o:p></o:p> </div> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: amd-gfx <<a href="mailto:amd-gfx-bounces@lists.freedesktop.org">amd-gfx-bounces@lists.freedesktop.org</a>> On Behalf Of Clements, John Sent: Friday, May 8, 2020 2:51 PM To: <a href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a>; Zhang, Hawking <<a href="mailto:Hawking.Zhang@amd.com">Hawking.Zhang@amd.com</a>> Subject: [PATCH] Series to disallow XGMI link power down during RAS XGMI error injection <o:p></o:p> </div> </div> <o:p> </o:p> [AMD Official Use Only - Internal Distribution Only]<o:p></o:p> <o:p> </o:p> Submitting 3 patches for review:<o:p></o:p> <ul style="margin-top:0in" type="disc"> <li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3">Added host to SMU FW cmd to enable/disable XGMI link power down<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3">Added DPM function for XGMI link power down control<o:p></o:p></li><li class="MsoListParagraph" style="margin-left:0in;mso-list:l0 level1 lfo3">Disable XGMI link power down prior to issuing a XGMI RAS error<o:p></o:p></li></ul> </div> </body> </html>