[PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Jithu Joseph]

Existing implementation limits IFS image to be loaded only from
a default file-name (ff-mm-ss.scan).

Change the semantics of the "reload" file. Writing "1" keeps the legacy
behavior to reload from the default "ff-mm-ss.scan" file, but now interpret
other strings as a filename to be loaded from the /lib/firmware/intel/ifs
directory.

Situations where multiple image files are helpful:
1. Test contents are larger than the memory reserved for IFS by BIOS
2. Increased test coverage
3. Custom test files to debug certain specific issues in field

Fix the below items in adjacent code
- Return error when ifs_image_sanity_check() fails in ifs_load_firmware()
- Correct documentation "ifs.0"->"ifs"

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h          | 11 +++++--
 drivers/platform/x86/intel/ifs/core.c         |  2 +-
 drivers/platform/x86/intel/ifs/load.c         | 30 +++++++++++++++----
 drivers/platform/x86/intel/ifs/sysfs.c        | 13 ++------
 .../ABI/testing/sysfs-platform-intel-ifs      |  5 +++-
 5 files changed, 41 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 73c8e91cf144..ee258e896dd0 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -39,7 +39,14 @@
  *   # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
  *
  * Similar to microcode, the current version of the scan tests is stored
- * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
+ * in a fixed location: /lib/firmware/intel/ifs/family-model-stepping.scan
+ *
+ * An alternate scan file can be loaded by writing its filename to the
+ * reload file::
+ *
+ *   # echo mytest > /sys/devices/virtual/misc/intel_ifs_0/reload
+ *
+ * The file will be loaded from the same directory, i.e. /lib/firmware/intel/ifs/mytest
  *
  * Running tests
  * -------------
@@ -225,7 +232,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, const char *file_name);
 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/core.c b/drivers/platform/x86/intel/ifs/core.c
index 27204e3d674d..306f599a8f5a 100644
--- a/drivers/platform/x86/intel/ifs/core.c
+++ b/drivers/platform/x86/intel/ifs/core.c
@@ -53,7 +53,7 @@ static int __init ifs_init(void)
 	if ((msrval & BIT(ifs_device.data.integrity_cap_bit)) &&
 	    !misc_register(&ifs_device.misc)) {
 		down(&ifs_sem);
-		ifs_load_firmware(ifs_device.misc.this_device);
+		ifs_load_firmware(ifs_device.misc.this_device, "1");
 		up(&ifs_sem);
 		return 0;
 	}
diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index d056617ddc85..6f49a2c593f4 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -232,17 +232,33 @@ 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}.
+ * in the folder /lib/firmware/intel/ifs/
  */
-void ifs_load_firmware(struct device *dev)
+int ifs_load_firmware(struct device *dev, const char *file_name)
 {
 	struct ifs_data *ifsd = ifs_get_data(dev);
+	bool default_name = false;
 	const struct firmware *fw;
-	char scan_path[32];
-	int ret;
+	char scan_path[64];
+	int ret = -EINVAL;
+	int file_name_len;
+
+	if ((kstrtobool(file_name, &default_name) == 0)) {
+		if (default_name)
+			snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan",
+				 boot_cpu_data.x86,
+				 boot_cpu_data.x86_model,
+				 boot_cpu_data.x86_stepping);
+	}
 
-	snprintf(scan_path, sizeof(scan_path), "intel/ifs/%02x-%02x-%02x.scan",
-		 boot_cpu_data.x86, boot_cpu_data.x86_model, boot_cpu_data.x86_stepping);
+	if (!default_name) {
+		if (strchr(file_name, '/'))
+			goto done;
+		file_name_len = strchrnul(file_name, '\n') - file_name;
+		if (snprintf(scan_path, sizeof(scan_path), "intel/ifs/%.*s",
+			     file_name_len, file_name) >= sizeof(scan_path))
+			goto done;
+	}
 
 	ret = request_firmware_direct(&fw, scan_path, dev);
 	if (ret) {
@@ -252,6 +268,7 @@ void ifs_load_firmware(struct device *dev)
 
 	if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
 		dev_err(dev, "ifs header sanity check failed\n");
+		ret = -EINVAL;
 		goto release;
 	}
 
@@ -263,4 +280,5 @@ 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..b4716b7d36aa 100644
--- a/drivers/platform/x86/intel/ifs/sysfs.c
+++ b/drivers/platform/x86/intel/ifs/sysfs.c
@@ -94,23 +94,16 @@ 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;
-
-
-	if (kstrtobool(buf, &res))
-		return -EINVAL;
-	if (!res)
-		return count;
+	int ret;
 
 	if (down_interruptible(&ifs_sem))
 		return -EINTR;
 
-	ifs_load_firmware(dev);
+	ret = ifs_load_firmware(dev, buf);
 
 	up(&ifs_sem);
 
-	return ifsd->loaded ? count : -ENODEV;
+	return ret  ? ret : count;
 }
 
 static DEVICE_ATTR_WO(reload);
diff --git a/Documentation/ABI/testing/sysfs-platform-intel-ifs b/Documentation/ABI/testing/sysfs-platform-intel-ifs
index 486d6d2ff8a0..560b85561eac 100644
--- a/Documentation/ABI/testing/sysfs-platform-intel-ifs
+++ b/Documentation/ABI/testing/sysfs-platform-intel-ifs
@@ -36,4 +36,7 @@ Date:		April 21 2022
 KernelVersion:	5.19
 Contact:	"Jithu Joseph" <jithu.joseph@intel.com>
 Description:	Write "1" (or "y" or "Y") to reload the IFS image from
-		/lib/firmware/intel/ifs/ff-mm-ss.scan.
+                the default location /lib/firmware/intel/ifs/ff-mm-ss.scan.
+                Alternatively write <file_name> to use a specific IFS image file.
+                In this case image from /lib/firmware/intel/ifs/<file_name> will
+                be loaded.

base-commit: 88084a3df1672e131ddc1b4e39eeacfd39864acf
-- 
2.25.1

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
> Existing implementation limits IFS image to be loaded only from
> a default file-name (ff-mm-ss.scan).
> 
> Change the semantics of the "reload" file. Writing "1" keeps the legacy
> behavior to reload from the default "ff-mm-ss.scan" file, but now interpret
> other strings as a filename to be loaded from the /lib/firmware/intel/ifs
> directory.
> 
> Situations where multiple image files are helpful:
> 1. Test contents are larger than the memory reserved for IFS by BIOS
> 2. Increased test coverage
> 3. Custom test files to debug certain specific issues in field

Ick, but now what namespace are you saying that path is in?  If you need
debugging stuff, then put the api/interface in debugfs and use it there,
don't overload the existing sysfs api to do something different here.

> Fix the below items in adjacent code
> - Return error when ifs_image_sanity_check() fails in ifs_load_firmware()
> - Correct documentation "ifs.0"->"ifs"

That should all be a separate patch, you and Tony know better than this.

{sigh}

greg k-h

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Joseph, Jithu]

On 7/8/2022 8:28 AM, Greg KH wrote:
> On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
>> Existing implementation limits IFS image to be loaded only from
>> a default file-name (ff-mm-ss.scan).
>>

> 
> Ick, but now what namespace are you saying that path is in?  If you need
> debugging stuff, then put the api/interface in debugfs and use it there,
> don't overload the existing sysfs api to do something different here.

The namespace related confusion could be because, the original commit message
was not using full path-names. The below write-up tries to be more clear on this

Existing implementation limits IFS images to be loaded only from
a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.

But there are situations where there may be multiple scan files
that can be run on a particular system stored in /lib/firmware/intel/ifs

E.g.
1. Because test contents are larger than the memory reserved for IFS by BIOS
2. To provide increased test coverage
3. Custom test files to debug certain specific issues in the field

Renaming each of these to ff-mm-ss.scan and then loading might be
possible in some environments. But on systems where /lib is read-only
this is not a practical solution.

Extend the semantics of the driver /sys/devices/virtual/misc/intel_ifs_0/reload
file:

  Writing "1" remains the legacy behavior to load from the default
  ff-mm-ss.scan file.

  Writing some other string is interpreted as a filename in
  /lib/firmware/intel/ifs to be loaded instead of the default file.



> 
>> Fix the below items in adjacent code
>> - Return error when ifs_image_sanity_check() fails in ifs_load_firmware()
>> - Correct documentation "ifs.0"->"ifs"
> 
> That should all be a separate patch, you and Tony know better than this.
> 

Noted ... will send this separately


Jithu

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Fri, Jul 08, 2022 at 11:34:40AM -0700, Joseph, Jithu wrote:
> 
> 
> On 7/8/2022 8:28 AM, Greg KH wrote:
> > On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
> >> Existing implementation limits IFS image to be loaded only from
> >> a default file-name (ff-mm-ss.scan).
> >>
> 
> > 
> > Ick, but now what namespace are you saying that path is in?  If you need
> > debugging stuff, then put the api/interface in debugfs and use it there,
> > don't overload the existing sysfs api to do something different here.
> 
> The namespace related confusion could be because, the original commit message
> was not using full path-names. The below write-up tries to be more clear on this
> 
> Existing implementation limits IFS images to be loaded only from
> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> 
> But there are situations where there may be multiple scan files
> that can be run on a particular system stored in /lib/firmware/intel/ifs
> 
> E.g.
> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> 2. To provide increased test coverage
> 3. Custom test files to debug certain specific issues in the field
> 
> Renaming each of these to ff-mm-ss.scan and then loading might be
> possible in some environments. But on systems where /lib is read-only
> this is not a practical solution.
> 
> Extend the semantics of the driver /sys/devices/virtual/misc/intel_ifs_0/reload
> file:
> 
>   Writing "1" remains the legacy behavior to load from the default
>   ff-mm-ss.scan file.
> 
>   Writing some other string is interpreted as a filename in
>   /lib/firmware/intel/ifs to be loaded instead of the default file.

Ick, you are overloading an existing sysfs file to do different things
based on random stuff.  This is a brand-new api that you are already
messing with in crazy ways.  Why not just revert the whole thing and
start over as obviously this was not tested well with real devices.

And what is wrong with a firmware file called '1'?  :)

thanks,

greg k-h

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/10/22 12:15, Greg KH wrote:
> On Fri, Jul 08, 2022 at 11:34:40AM -0700, Joseph, Jithu wrote:
>>
>>
>> On 7/8/2022 8:28 AM, Greg KH wrote:
>>> On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
>>>> Existing implementation limits IFS image to be loaded only from
>>>> a default file-name (ff-mm-ss.scan).
>>>>
>>
>>>
>>> Ick, but now what namespace are you saying that path is in?  If you need
>>> debugging stuff, then put the api/interface in debugfs and use it there,
>>> don't overload the existing sysfs api to do something different here.
>>
>> The namespace related confusion could be because, the original commit message
>> was not using full path-names. The below write-up tries to be more clear on this
>>
>> Existing implementation limits IFS images to be loaded only from
>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>>
>> But there are situations where there may be multiple scan files
>> that can be run on a particular system stored in /lib/firmware/intel/ifs
>>
>> E.g.
>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
>> 2. To provide increased test coverage
>> 3. Custom test files to debug certain specific issues in the field
>>
>> Renaming each of these to ff-mm-ss.scan and then loading might be
>> possible in some environments. But on systems where /lib is read-only
>> this is not a practical solution.
>>
>> Extend the semantics of the driver /sys/devices/virtual/misc/intel_ifs_0/reload
>> file:
>>
>>   Writing "1" remains the legacy behavior to load from the default
>>   ff-mm-ss.scan file.
>>
>>   Writing some other string is interpreted as a filename in
>>   /lib/firmware/intel/ifs to be loaded instead of the default file.
> 
> Ick, you are overloading an existing sysfs file to do different things
> based on random stuff.  This is a brand-new api that you are already
> messing with in crazy ways.  Why not just revert the whole thing and
> start over as obviously this was not tested well with real devices.
> 
> And what is wrong with a firmware file called '1'?  :)

Actually the Intel IFS stuff has landed in 5.19-rc# so it is
a bit late(ish) for dropping it now.

But I do agree overloading the reload attribute is ugly,
why not just add a new /sys/devices/virtual/misc/intel_ifs_0/filename
rw sysfs attribute and use that to allow the user to specify a
filename to load different from the default one. Then to load the
<whatever> test firmware the user can do:

echo "<whatever>" > /sys/devices/virtual/misc/intel_ifs_0/filename
echo > /sys/devices/virtual/misc/intel_ifs_0/reload

that seems much cleaner to me ?

Regards,

Hans

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Greg KH]

On Sun, Jul 10, 2022 at 03:42:29PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/10/22 12:15, Greg KH wrote:
> > On Fri, Jul 08, 2022 at 11:34:40AM -0700, Joseph, Jithu wrote:
> >>
> >>
> >> On 7/8/2022 8:28 AM, Greg KH wrote:
> >>> On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
> >>>> Existing implementation limits IFS image to be loaded only from
> >>>> a default file-name (ff-mm-ss.scan).
> >>>>
> >>
> >>>
> >>> Ick, but now what namespace are you saying that path is in?  If you need
> >>> debugging stuff, then put the api/interface in debugfs and use it there,
> >>> don't overload the existing sysfs api to do something different here.
> >>
> >> The namespace related confusion could be because, the original commit message
> >> was not using full path-names. The below write-up tries to be more clear on this
> >>
> >> Existing implementation limits IFS images to be loaded only from
> >> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
> >>
> >> But there are situations where there may be multiple scan files
> >> that can be run on a particular system stored in /lib/firmware/intel/ifs
> >>
> >> E.g.
> >> 1. Because test contents are larger than the memory reserved for IFS by BIOS
> >> 2. To provide increased test coverage
> >> 3. Custom test files to debug certain specific issues in the field
> >>
> >> Renaming each of these to ff-mm-ss.scan and then loading might be
> >> possible in some environments. But on systems where /lib is read-only
> >> this is not a practical solution.
> >>
> >> Extend the semantics of the driver /sys/devices/virtual/misc/intel_ifs_0/reload
> >> file:
> >>
> >>   Writing "1" remains the legacy behavior to load from the default
> >>   ff-mm-ss.scan file.
> >>
> >>   Writing some other string is interpreted as a filename in
> >>   /lib/firmware/intel/ifs to be loaded instead of the default file.
> > 
> > Ick, you are overloading an existing sysfs file to do different things
> > based on random stuff.  This is a brand-new api that you are already
> > messing with in crazy ways.  Why not just revert the whole thing and
> > start over as obviously this was not tested well with real devices.
> > 
> > And what is wrong with a firmware file called '1'?  :)
> 
> Actually the Intel IFS stuff has landed in 5.19-rc# so it is
> a bit late(ish) for dropping it now.

We can mark it BROKEN right now before -final happens as it seems that
the api in 5.19-rc is not correct for its users.

Perhaps we should do that now to give people the chance to get it right?

thanks,

greg k-h

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi Greg,

On 7/10/22 15:53, Greg KH wrote:
> On Sun, Jul 10, 2022 at 03:42:29PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/10/22 12:15, Greg KH wrote:
>>> On Fri, Jul 08, 2022 at 11:34:40AM -0700, Joseph, Jithu wrote:
>>>>
>>>>
>>>> On 7/8/2022 8:28 AM, Greg KH wrote:
>>>>> On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
>>>>>> Existing implementation limits IFS image to be loaded only from
>>>>>> a default file-name (ff-mm-ss.scan).
>>>>>>
>>>>
>>>>>
>>>>> Ick, but now what namespace are you saying that path is in?  If you need
>>>>> debugging stuff, then put the api/interface in debugfs and use it there,
>>>>> don't overload the existing sysfs api to do something different here.
>>>>
>>>> The namespace related confusion could be because, the original commit message
>>>> was not using full path-names. The below write-up tries to be more clear on this
>>>>
>>>> Existing implementation limits IFS images to be loaded only from
>>>> a default file-name /lib/firmware/intel/ifs/ff-mm-ss.scan.
>>>>
>>>> But there are situations where there may be multiple scan files
>>>> that can be run on a particular system stored in /lib/firmware/intel/ifs
>>>>
>>>> E.g.
>>>> 1. Because test contents are larger than the memory reserved for IFS by BIOS
>>>> 2. To provide increased test coverage
>>>> 3. Custom test files to debug certain specific issues in the field
>>>>
>>>> Renaming each of these to ff-mm-ss.scan and then loading might be
>>>> possible in some environments. But on systems where /lib is read-only
>>>> this is not a practical solution.
>>>>
>>>> Extend the semantics of the driver /sys/devices/virtual/misc/intel_ifs_0/reload
>>>> file:
>>>>
>>>>   Writing "1" remains the legacy behavior to load from the default
>>>>   ff-mm-ss.scan file.
>>>>
>>>>   Writing some other string is interpreted as a filename in
>>>>   /lib/firmware/intel/ifs to be loaded instead of the default file.
>>>
>>> Ick, you are overloading an existing sysfs file to do different things
>>> based on random stuff.  This is a brand-new api that you are already
>>> messing with in crazy ways.  Why not just revert the whole thing and
>>> start over as obviously this was not tested well with real devices.
>>>
>>> And what is wrong with a firmware file called '1'?  :)
>>
>> Actually the Intel IFS stuff has landed in 5.19-rc# so it is
>> a bit late(ish) for dropping it now.
> 
> We can mark it BROKEN right now before -final happens as it seems that
> the api in 5.19-rc is not correct for its users.
> 
> Perhaps we should do that now to give people the chance to get it right?

That is a good idea. I've just send out a patch doing that.

I plan to submit one last pdx86 fixes pull-req to Linus once rc6 is out
(prepping it now and want to give the builders some time to build test it).

I'll include this in this fixes pull-req.

Regards,

Hans

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Joseph, Jithu]

On 7/10/2022 7:08 AM, Hans de Goede wrote:
> Hi Greg,
> 
> On 7/10/22 15:53, Greg KH wrote:
>> On Sun, Jul 10, 2022 at 03:42:29PM +0200, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 7/10/22 12:15, Greg KH wrote:
>>>> On Fri, Jul 08, 2022 at 11:34:40AM -0700, Joseph, Jithu wrote:
>>>>>
>>>>>
>>>>> On 7/8/2022 8:28 AM, Greg KH wrote:
>>>>>> On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
>>>>>>> Existing implementation limits IFS image to be loaded only from

>>>>
>>>> Ick, you are overloading an existing sysfs file to do different things
>>>> based on random stuff.  This is a brand-new api that you are already
>>>> messing with in crazy ways.  Why not just revert the whole thing and
>>>> start over as obviously this was not tested well with real devices.
>>>>
>>>> And what is wrong with a firmware file called '1'?  :)
>>>
>>> Actually the Intel IFS stuff has landed in 5.19-rc# so it is
>>> a bit late(ish) for dropping it now.
>>
>> We can mark it BROKEN right now before -final happens as it seems that
>> the api in 5.19-rc is not correct for its users.
>>
>> Perhaps we should do that now to give people the chance to get it right?
> 
> That is a good idea. I've just send out a patch doing that.
> 
> I plan to submit one last pdx86 fixes pull-req to Linus once rc6 is out
> (prepping it now and want to give the builders some time to build test it).
> 
> I'll include this in this fixes pull-req.
> 

I did send a v2 just now, which removes treating 1 specially. Not sure
if it is too late, but just wanted to give it a shot


Jithu

Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS image

[Hans de Goede]

Hi,

On 7/10/22 18:04, Joseph, Jithu wrote:
> 
> 
> On 7/10/2022 7:08 AM, Hans de Goede wrote:
>> Hi Greg,
>>
>> On 7/10/22 15:53, Greg KH wrote:
>>> On Sun, Jul 10, 2022 at 03:42:29PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 7/10/22 12:15, Greg KH wrote:
>>>>> On Fri, Jul 08, 2022 at 11:34:40AM -0700, Joseph, Jithu wrote:
>>>>>>
>>>>>>
>>>>>> On 7/8/2022 8:28 AM, Greg KH wrote:
>>>>>>> On Fri, Jul 08, 2022 at 08:19:38AM -0700, Jithu Joseph wrote:
>>>>>>>> Existing implementation limits IFS image to be loaded only from
> 
>>>>>
>>>>> Ick, you are overloading an existing sysfs file to do different things
>>>>> based on random stuff.  This is a brand-new api that you are already
>>>>> messing with in crazy ways.  Why not just revert the whole thing and
>>>>> start over as obviously this was not tested well with real devices.
>>>>>
>>>>> And what is wrong with a firmware file called '1'?  :)
>>>>
>>>> Actually the Intel IFS stuff has landed in 5.19-rc# so it is
>>>> a bit late(ish) for dropping it now.
>>>
>>> We can mark it BROKEN right now before -final happens as it seems that
>>> the api in 5.19-rc is not correct for its users.
>>>
>>> Perhaps we should do that now to give people the chance to get it right?
>>
>> That is a good idea. I've just send out a patch doing that.
>>
>> I plan to submit one last pdx86 fixes pull-req to Linus once rc6 is out
>> (prepping it now and want to give the builders some time to build test it).
>>
>> I'll include this in this fixes pull-req.
>>
> 
> I did send a v2 just now, which removes treating 1 specially. Not sure
> if it is too late, but just wanted to give it a shot

The v2 patch does look better to me, thanks.

But IMHO it is not good idea to fix userspace API issues during or
after rc6. So lets keep this marked as broken in Kconfig for 5.19 final
and then we can get something like v2 merged into 5.20-rc1 in a couple
of weeks and then also remove the broken marking.

For enterprise distros to be able to backport this what matters is it
being in Torvald's master branch, so from that pov this just delays
things a couple of weeks. Which is well worth it IMHO to give us some more
time to get the userspace API right.

Regards,

Hans

[PATCH 0/2] Two fixes for IFS

[Tony Luck]

Hans ... if you are pushing to Linus to add "BROKEN", then could
you include these two patches to make it fractionally less broken?

-Tony

Jithu Joseph (2):
  Documentation: Correct IFS reload documentation
  platform/x86/intel/ifs: return error on load failure

 drivers/platform/x86/intel/ifs/ifs.h  | 2 +-
 drivers/platform/x86/intel/ifs/load.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.35.3

[PATCH 1/2] Documentation: Correct IFS reload documentation

[Tony Luck]

From: Jithu Joseph <jithu.joseph@intel.com>

The location where the scan image is stored is incorrect
in the current documentation. Fix this.

Fixes: 34604d289167("Documentation: In-Field Scan")
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/platform/x86/intel/ifs/ifs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/ifs/ifs.h b/drivers/platform/x86/intel/ifs/ifs.h
index 73c8e91cf144..622a246b62cb 100644
--- a/drivers/platform/x86/intel/ifs/ifs.h
+++ b/drivers/platform/x86/intel/ifs/ifs.h
@@ -39,7 +39,7 @@
  *   # echo 1 > /sys/devices/virtual/misc/intel_ifs_0/reload
  *
  * Similar to microcode, the current version of the scan tests is stored
- * in a fixed location: /lib/firmware/intel/ifs.0/family-model-stepping.scan
+ * in a fixed location: /lib/firmware/intel/ifs/family-model-stepping.scan
  *
  * Running tests
  * -------------
-- 
2.35.3

[PATCH 2/2] platform/x86/intel/ifs: return error on load failure

[Tony Luck]

From: Jithu Joseph <jithu.joseph@intel.com>

A bug in ifs_load_firmware() error path will make it return
SUCCESS in the event of failure.

If ifs_image_sanity_check() fails, then "ret" is still zero (from
the earlier successful call to request_firmware_direct().

Reinitialize the return variable with appropriate error code.

Fixes: 684ec215706d4 ("platform/x86/intel/ifs: Authenticate and copy to secured memory")
Signed-off-by: Jithu Joseph <jithu.joseph@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 drivers/platform/x86/intel/ifs/load.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/intel/ifs/load.c b/drivers/platform/x86/intel/ifs/load.c
index d056617ddc85..3edcc570f1fe 100644
--- a/drivers/platform/x86/intel/ifs/load.c
+++ b/drivers/platform/x86/intel/ifs/load.c
@@ -252,6 +252,7 @@ void ifs_load_firmware(struct device *dev)
 
 	if (!ifs_image_sanity_check(dev, (struct microcode_header_intel *)fw->data)) {
 		dev_err(dev, "ifs header sanity check failed\n");
+		ret = -EINVAL;
 		goto release;
 	}
 
-- 
2.35.3

Re: [PATCH 0/2] Two fixes for IFS

[Hans de Goede]

Hi Tony,

On 7/10/22 20:25, Tony Luck wrote:
> Hans ... if you are pushing to Linus to add "BROKEN", then could
> you include these two patches to make it fractionally less broken?

Since the driver is going to be marked as BROKEN now anyways,
I don't see much value in getting these send last minute to Linus.

At this point in the kernel cycle (rc6 will be out in a couple
of hours likely) we really should avoid churn as much as possible.

Regards,

Hans

Re: [PATCH 1/2] Documentation: Correct IFS reload documentation

[Greg KH]

On Sun, Jul 10, 2022 at 11:25:20AM -0700, Tony Luck wrote:
> From: Jithu Joseph <jithu.joseph@intel.com>
> 
> The location where the scan image is stored is incorrect
> in the current documentation. Fix this.
> 
> Fixes: 34604d289167("Documentation: In-Field Scan")

Our tools will complain about this line, please use the correct format
so you don't get grumpy emails from the linux-next maintainer.

thanks,

greg k-h