Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <d227e106-1766-c51e-397d-209c1f880a65@amd.com>
Date:   Thu, 5 Jan 2023 10:59:22 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v6 05/45] drm/amd: Add a new helper for loading/validating
 microcode
Content-Language: en-US
To:     Mario Limonciello <mario.limonciello@amd.com>,
        Alex Deucher <alexander.deucher@amd.com>,
        linux-kernel@vger.kernel.org
Cc:     Javier Martinez Canillas <javierm@redhat.com>,
        Carlos Soriano Sanchez <csoriano@redhat.com>,
        amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, christian.koenig@amd.com,
        "Pan, Xinhui" <Xinhui.Pan@amd.com>
References: <20230105034327.1439-1-mario.limonciello@amd.com>
 <20230105034327.1439-6-mario.limonciello@amd.com>
 <804491f0-6e83-dae5-f108-f80950838a52@amd.com>
 <b3304913-e6f4-3f87-e4f9-b31374682d2a@amd.com>
From:   "Lazar, Lijo" <lijo.lazar@amd.com>
In-Reply-To: <b3304913-e6f4-3f87-e4f9-b31374682d2a@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SWUxeDhVQnNkM1JNSWVONkdUNkpXWFVMRE5GZCtOclRvODNmMG1pSHdQU045?=
 =?utf-8?B?REJUelZEM29PMDZWR1pBSDZ1aEJmbWh0dnM2Sm84cE9nai81RGpKTERHN203?=
 =?utf-8?B?YS9zOEFUTTFqTkZiVHJOUFdLZk9ib21CcTlpNGhtbmFoakg3Y3kzWW9NMDh0?=
 =?utf-8?B?L3lJdEh6dzlaWEpadW1aaWdlRHc5dWtLUnc0T0ZncHZmMjdhdERBaHNGZFBu?=
 =?utf-8?B?OER4VEl1QlBEOEhTd0c2OWEzbTlPVkxyYmRmSGRnVGx6MWtOTWxZc2dBUFlN?=
 =?utf-8?B?TGg3d1d0eGYxNnA3M1pZM1c1QkJpZVBzdnNSL0hnYjhBcmpCclFoWVZUVXZU?=
 =?utf-8?B?bmVJUytHa1BpbFBVemJwWlhJTHRUMTBDRU1DRDRrNk41TU1YTTNmUGdUOVVm?=
 =?utf-8?B?MGdjVnVoVC9aVWxpQVY2RUtIaGFJODBldzI2dVloKy9HKzVSdmZKSFBZS3Yv?=
 =?utf-8?B?RlEwa2xYdTdoZmNOYVJqdTl1RksyT0wwbmJSS0N1aVhiSnN2Rm14dS9DZzdP?=
 =?utf-8?B?NW5tTW92cGp3STBIVXB6eTZvRmZUOENGVTRSbUVEUFhLTm5iWlhDRVdtNTVZ?=
 =?utf-8?B?dEhEZDlOaFdTRTI0TzlzUE1LM1lhTDZXNVljaDVtU05BTms3UmlNczJjWFpK?=
 =?utf-8?B?aFZmdUxJOHgySUt1T1FIRE1sblZRMGFsYzZWY2dteW03R3pTUmRNejdMd1pB?=
 =?utf-8?B?SVZYaitXUHlNU080ZjRzcGh1dlFVekVmWlRyNlgyOHdJeEtVeVcyeFlHR01H?=
 =?utf-8?B?aDUwZndPMldQMHk3bzVWNUZHbUdpVDRvT1R1d0NjNUlFUzBqK3ZaSU1CdWFs?=
 =?utf-8?B?NHh6c29TNDhnT203Mnl3VWFBWVVYNzJWcWlFVVVianhlVXNoM1ZOVm03M0FY?=
 =?utf-8?B?Y2ZxMEM3NVU5OTQ5K1dMTSs4M3lxa1VIV1dob3c5aHQvc3hSay82VGpsRk5s?=
 =?utf-8?B?UUgzZ3kzbmRSN0s4MVBhMFlpbmUrcmVsUnFIdGFzeGhvVFgwNi9FS3JEdS9W?=
 =?utf-8?B?VEw2cFFmTVpFOWxvQmpsRlhKZVFzS3NHdDB0WTVObWNRRFlqbWxBT2dZSXRw?=
 =?utf-8?B?WGlTWFRuTEdFSHhGbktMZWFjOExRWVFveVV4eXlCakI3QU43OC81eVNwZ1ZP?=
 =?utf-8?B?dlh3d0tVREJ4ajNiZ1l4ckhVVjZCeU5WVnNEcW5LN2R6TFhQUnQ0NFRnOWFl?=
 =?utf-8?B?RVdHaGwxNTJ0TW5LejRGTm55RjdUWllEU1dXZTBsTHhKcnd3YUc3YjNGc1Zl?=
 =?utf-8?B?bkJWOXI5NFYyWXp3ZS9URzkxMDNEOGpyd2NxQjhBTEtKVGJlL2VXY25jZTdI?=
 =?utf-8?B?OUFGWGpSOHNFbEdKTHdnd3JGcCtuSnVFUW1pN1Y0eTZUQmRZczd6dE9nNmlO?=
 =?utf-8?B?VDlFNnMvTjRtVUpyVFFld0E5MTFvZnE5SmdZK0NjYzVCeWdJRDlkeHAvSWh1?=
 =?utf-8?B?SkJEUW1vMU1TWFdJYnBzdXUrVXNTaXY2VUxGaDdFNVBTcE5VbGhNTlliTDlZ?=
 =?utf-8?B?djA0NFJkSys4VnNhWkdTc1p3enVTS0hoRU5iRUpIdWRSRllLQzJHeDZISEhz?=
 =?utf-8?B?aG5zSFdza2hoVkxqK1RRUUVGZVU3QlhTWVNmbEt5RFBqa1NTMGt1YXVQWnM0?=
 =?utf-8?B?T2VkWGY1R2U3NDNSQkM0V1BibnJLZHVtQ1U3TlYrSEwwSndlWU5idGllNFo5?=
 =?utf-8?B?aUJvNjZxeHVlbWxyOVN0QTlsSHREd2E2aGVIbmYrdjNJTFZIL3NIdUFMSklT?=
 =?utf-8?B?MFpJMEJSc0t6c0FuaTBoMDBaaGNWRHp3SmppYlFRUjZndVdld1k5RG84R0c0?=
 =?utf-8?B?WmhwcGd5andmYlRqd3EzOUxBNCtVeXp0bCtlaTFGdmprQnpTaE5qSi8rZlMz?=
 =?utf-8?B?MFpZQWtTZkp0Yy9XMTNwRDZwY3V1NUJWaTA5eXF4SCtrQU9kaC9wbVZnNmlv?=
 =?utf-8?B?QjdGcVg5VEZVaE13LzhBVzVuVEtvUmt1SXZoOHRrY1EzOGE2WlhIVVUzQWo0?=
 =?utf-8?B?NjJxdEhBeFhyaUtNRWp2M0x2K29NdUhBRDhvVWxoNE5ydkhvZDJOdi90UTBy?=
 =?utf-8?B?TWhqT1FsSEZFd1N2c3hEb2NKcHdHYWltSW5GY04vQ1U3V0JmaUl6UWw1OWRv?=
 =?utf-8?Q?3mw1q96YBF/Ol/6O1FqhKk53g?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2015e77e-d5be-46b8-3dcd-08daeeddd554
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB4614.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2023 05:29:36.3722
 (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: mOJ9WreVQJNRIFZgbGpylVk/B0hlsG1zQJVX7ArRo/AtZuv0B0p3Aq9htjqhFyjQ
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR12MB6559
X-Spam-Status: No, score=-3.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/5/2023 10:53 AM, Mario Limonciello wrote:
> On 1/4/23 23:07, Lazar, Lijo wrote:
>>
>>
>> On 1/5/2023 9:12 AM, Mario Limonciello wrote:
>>> All microcode runs a basic validation after it's been loaded. Each
>>> IP block as part of init will run both.
>>>
>>> Introduce a wrapper for request_firmware and amdgpu_ucode_validate.
>>> This wrapper will also remap any error codes from request_firmware
>>> to -ENODEV.  This is so that early_init will fail if firmware couldn't
>>> be loaded instead of the IP block being disabled.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>> v5->v6:
>>>   * Fix argument to be ** not *
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c | 36 +++++++++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h |  3 ++
>>>   2 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>> index eafcddce58d3..8ebfec12da87 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
>>> @@ -1312,3 +1312,39 @@ void amdgpu_ucode_ip_version_decode(struct 
>>> amdgpu_device *adev, int block_type,
>>>       snprintf(ucode_prefix, len, "%s_%d_%d_%d", ip_name, maj, min, 
>>> rev);
>>>   }
>>> +
>>> +/*
>>> + * amdgpu_ucode_request - Fetch and validate amdgpu microcode
>>> + *
>>> + * @adev: amdgpu device
>>> + * @fw: pointer to load firmware to
>>> + * @fw_name: firmware to load
>>> + *
>>> + * This is a helper that will use request_firmware and 
>>> amdgpu_ucode_validate
>>> + * to load and run basic validation on firmware. If the load fails, 
>>> remap
>>> + * the error code to -ENODEV, so that early_init functions will fail 
>>> to load.
>>> + */
>>> +int amdgpu_ucode_request(struct amdgpu_device *adev, const struct 
>>> firmware **fw,
>>> +             const char *fw_name)
>>> +{
>>> +    int err = request_firmware(fw, fw_name, adev->dev);
>>> +
>>> +    if (err)
>>> +        return -ENODEV;
>>> +    err = amdgpu_ucode_validate(*fw);
>>> +    if (err)
>>> +        dev_dbg(adev->dev, "\"%s\" failed to validate\n", fw_name);
>>> +
>>
>> Missed this earlier. If validate fails, shouldn't this undo the 
>> request operation by calling release?
> 
> Actually that was original design, but there is one place in the 
> codebase that expects that ucode validation can fail, and so leave the 
> evaluate of error code and cleanup outside of helper.
> 

I see. Does request_firmware assure that fw pointer be always NULL if it 
fails? Or should that be done here if request_ fails? In subsequent 
patches, I see clients calling release without checking what caused the 
failure.

Thanks,
Lijo

>>
>> Thanks,
>> Lijo
>>
>>> +    return err;
>>> +}
>>> +
>>> +/*
>>> + * amdgpu_ucode_release - Release firmware microcode
>>> + *
>>> + * @fw: pointer to firmware to release
>>> + */
>>> +void amdgpu_ucode_release(const struct firmware **fw)
>>> +{
>>> +    release_firmware(*fw);
>>> +    *fw = NULL;
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
>>> index 552e06929229..848579d4988b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.h
>>> @@ -544,6 +544,9 @@ void amdgpu_ucode_print_sdma_hdr(const struct 
>>> common_firmware_header *hdr);
>>>   void amdgpu_ucode_print_psp_hdr(const struct common_firmware_header 
>>> *hdr);
>>>   void amdgpu_ucode_print_gpu_info_hdr(const struct 
>>> common_firmware_header *hdr);
>>>   int amdgpu_ucode_validate(const struct firmware *fw);
>>> +int amdgpu_ucode_request(struct amdgpu_device *adev, const struct 
>>> firmware **fw,
>>> +             const char *fw_name);
>>> +void amdgpu_ucode_release(const struct firmware **fw);
>>>   bool amdgpu_ucode_hdr_version(union amdgpu_firmware_header *hdr,
>>>                   uint16_t hdr_major, uint16_t hdr_minor);
>