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

From: Hans de Goede
Date: Sun Jul 10 2022 - 09:45:20 EST


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 ?