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

["Joseph, Jithu" <jithu.joseph@intel.com>]

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