linux-kernel - Re: [PATCH 02/14] platform/x86/intel/ifs: Propagate load failure error code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 24 Oct 2022 16:17:01 -0700
From:   "Joseph, Jithu" <jithu.joseph@...el.com>
To:     Sohil Mehta <sohil.mehta@...el.com>
CC:     <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
        <dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
        <gregkh@...uxfoundation.org>, <ashok.raj@...el.com>,
        <tony.luck@...el.com>, <linux-kernel@...r.kernel.org>,
        <platform-driver-x86@...r.kernel.org>, <patches@...ts.linux.dev>,
        <ravi.v.shankar@...el.com>, <thiago.macieira@...el.com>,
        <athenas.jimenez.gonzalez@...el.com>, <hdegoede@...hat.com>,
        <markgross@...nel.org>
Subject: Re: [PATCH 02/14] platform/x86/intel/ifs: Propagate load failure
 error code



On 10/24/2022 3:52 PM, Sohil Mehta wrote:
> Hi Jithu,
> 
> On 10/21/2022 1:34 PM, Jithu Joseph wrote:
>> Existing implementation was returning fixed error code to user space
>> regardless of the load failure encountered.
>>
> 
> The tense is a bit confusing here. Also, 'Existing implementation' is typically implied and unnecessary. Would something like this be better?
> 
> The reload operation returns a fixed error code to user space regardless of the load failure encountered.

Thanks Sohil for the review, will reword as you suggest above

> 
> Modify..
> 
>> Modify this to propagate the actual error code to user space.
>>
> 
> ...
> 
>> diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
>> index d056617ddc85..ebaa1d6a2b18 100644
>> --- a/drivers/platform/x86/intel/ifs/load.c
>> +++ b/drivers/platform/x86/intel/ifs/load.c
>> @@ -234,7 +234,7 @@ static bool ifs_image_sanity_check(struct device *dev, const struct microcode_he
>>    * Load ifs image. Before loading ifs module, the ifs image must be located
>>    * in /lib/firmware/intel/ifs and named as {family/model/stepping}.{testname}.
>>    */
>> -void ifs_load_firmware(struct device *dev)
>> +int ifs_load_firmware(struct device *dev)
>>   {
>>       struct ifs_data *ifsd = ifs_get_data(dev);
>>       const struct firmware *fw;
>> @@ -263,4 +263,6 @@ void ifs_load_firmware(struct device *dev)
>>       release_firmware(fw);
>>   done:
>>       ifsd->loaded = (ret == 0);
>> +
>> +    return ret;
>>   }

This change is still needed by the new code, more below

>> diff --git a/drivers/platform/x86/intel/ifs/sysfs.c b/drivers/platform/x86/intel/ifs/sysfs.c
>> index 37d8380d6fa8..4af4e1bea98d 100644
>> --- a/drivers/platform/x86/intel/ifs/sysfs.c
>> +++ b/drivers/platform/x86/intel/ifs/sysfs.c
>> @@ -94,9 +94,8 @@ static ssize_t reload_store(struct device *dev,
>>                   struct device_attribute *attr,
>>                   const char *buf, size_t count)
>>   {
>> -    struct ifs_data *ifsd = ifs_get_data(dev);
>>       bool res;
>> -
>> +    int rc;
>>   
> 
> Does rc refer to return code? The other IFS functions like above use the commonly used 'ret' variable. Any specific reason for the inconsistency?
> 
> Also, patch 11 completely removes the reload_store() function. Should we avoid a separate patch to fix something that is going to be removed soon? Would re-ordering the patches help in that regard?

You are right that reload is removed subsequently, only the ifs_load_firmware() part above is needed for the new code . Will move the above to a new patch between current patches 11 and 12 (and drop this patch)



Jithu