| Message ID | 20221021203413.1220137-3-jithu.joseph@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | IFS multi test image support and misc changes | expand |
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. 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; > } > 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? > if (kstrtobool(buf, &res)) > return -EINVAL; > @@ -106,11 +105,11 @@ static ssize_t reload_store(struct device *dev, > if (down_interruptible(&ifs_sem)) > return -EINTR; > > - ifs_load_firmware(dev); > + rc = ifs_load_firmware(dev); > > up(&ifs_sem); > > - return ifsd->loaded ? count : -ENODEV; > + return (rc == 0) ? count : rc; > } > > static DEVICE_ATTR_WO(reload); Sohil
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
diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h index 73c8e91cf144..782bcc039ddb 100644 --- a/drivers/platform/x86/intel/ifs/ifs.h +++ b/drivers/platform/x86/intel/ifs/ifs.h @@ -225,7 +225,7 @@ static inline struct ifs_data *ifs_get_data(struct device *dev) return &d->data; } -void ifs_load_firmware(struct device *dev); +int ifs_load_firmware(struct device *dev); int do_core_test(int cpu, struct device *dev); const struct attribute_group **ifs_get_groups(void); 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; } 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; if (kstrtobool(buf, &res)) return -EINVAL; @@ -106,11 +105,11 @@ static ssize_t reload_store(struct device *dev, if (down_interruptible(&ifs_sem)) return -EINTR; - ifs_load_firmware(dev); + rc = ifs_load_firmware(dev); up(&ifs_sem); - return ifsd->loaded ? count : -ENODEV; + return (rc == 0) ? count : rc; } static DEVICE_ATTR_WO(reload);