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
