Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 10 Jul 2022 15:53:05 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     "Joseph, Jithu" <jithu.joseph@intel.com>, markgross@kernel.org,
        ashok.raj@intel.com, tony.luck@intel.com, ravi.v.shankar@intel.com,
        linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        patches@lists.linux.dev
Subject: Re: [PATCH] platform/x86/intel/ifs: Allow non-default names for IFS
 image
Message-ID: <YsrZwc9P34lICS99@kroah.com>
References: <20220708151938.986530-1-jithu.joseph@intel.com>
 <YshNAh6awfPFmxzU@kroah.com>
 <33a6193e-1084-ae5f-1f80-232274f71bd0@intel.com>
 <Ysqm1BRbCTtWfAcW@kroah.com>
 <79eae42f-50ca-c23c-9fd0-8c356b2d3783@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <79eae42f-50ca-c23c-9fd0-8c356b2d3783@redhat.com>
Precedence: bulk

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