Re: [PATCH 2/7] firmware: add offset to request_firmware_into_buf

[Scott Branden]

Hi Luis,

Thanks for helping on this.

Enjoy your time off an we can work on it when you're back.

comments below.


On 2019-08-23 8:47 a.m., Luis Chamberlain wrote:
> On Thu, Aug 22, 2019 at 04:30:37PM -0700, Scott Branden wrote:
> > On 2019-08-22 2:12 p.m., Luis Chamberlain wrote:
> > > On Thu, Aug 22, 2019 at 01:07:41PM -0700, Scott Branden wrote:
> > > > On 2019-08-22 12:47 p.m., Luis Chamberlain wrote:
> > > > > This implies you having to change the other callers, and while currently
> > > > > our list of drivers is small,
> > > > Yes, the list is small, very small.
> > > >
> > > > There is a single driver making a call to the existing API.
> > > >
> > > > And, the maintainer of that driver wanted
> > > > to start utilizing my enhanced API instead of the current API.
> > > You mean in the near term future? Your change makes it use the full file.
> > > Just checking.
> > The change in the patch keeps the existing functionality in the
> BTW for some reason your mailer keeps adding new lines per each line. I
> trim them below. Also for future emails please Cc:
>
>    Mimi Zohar <zohar@xxxxxxxxxxxxx>
>
> As she'll be interested in some of this from the IMA security perspective.
>
> > qcom mdt_loader by reading the full file using the enhanced api.
> > I don't know when Bjorn will switch to use the partial firmware load:
> >
> > https://lkml.org/lkml/2019/5/27/9
> OK I see he did he liked the approach. OK thanks! This will make
> evolutions much easier.
>
> > > > As such I think it is very reasonable to update the API right now.
> > > I'd prefer to see it separate, and we fix the race *before* we introduce
> > > the new functionality. I'll be poking at that shortly but I should note
> > > that I leave on vacation this weekend and won't be back for a good while.
> > > I already have an idea of how to approach this.
> > >
> > > When the current user want to use the new API it can do so, and then we
> > > just kill the older caller.
> > We can kill the older api right now as my patch in qcom mdt_loader
> > calls the new API which allows reading of full or partial files?
> Yes its possible, but more on this below.
>
> > > > > following the history of the firmware API
> > > > > and the long history of debate of *how* we should evolve its API, its
> > > > > preferred we add yet another new caller for this functionality. So
> > > > > please add a new caller, and use EXPORT_SYMBOL_GPL().
> > > > >
> > > > > And while at it, pleaase use firmware_request_*() as the prefix, as we
> > > > > have want to use that as the instilled prefix. We have yet to complete
> > > > > the rename of the others older callers but its just a matter of time.
> > > > >
> > > > > So something like: firmware_request_into_buf_offset()
> > > > I would prefer to rename the API at this time given there is only a single
> > > > user.
> > > >
> > > > Otherwise I would need to duplicate quite a bit in the test code to support
> > > > testing the single user of the old api and then enhanced API.
> > > > Or, I can leave existing API in place and change the test case to
> > > > just test the enhanced API to keep things simpler in the test code?
> > > If the new user is going to move to the API once available I will be
> > > happy to then leave out testing for the older API. That would make
> > > sense.
> > I have switched the single user of the existing api to the new
> > API in the patch already?
> Right, but in the new approach you'd use a newer function name with
> the new feature.

Yes, I will send a new version with a new function name.

firmware_request_into_buf() is more appropriate than 
firmware_request_into_buf_offset() though.

The function accepts both partial or full file requests with or without 
an offset into the file.

> > And both full and partial reads using the new API are tested with this
> > patch series. If you really insist on keeping the old API for a
> > single user I can drop that change from the patch series and have the
> > old request_firmware_api call simply be a wrapper calling the new API.
> Yes please.
Sure, if you want me to remove the change to the existing qcom driver to 
keep using the old api as well I'll do so.
> > > But if you do want to keep testing for the old API, and allow an easy
> > > removal for it on the test driver, wouldn't a function pointer suffice
> > > for which API call to use based on a boolean?
> > >
> > > But yeah if we're going to abandon the old mechanism I'm happy to skip
> > > its testing.
> > We can skip right now then. As enhanced API is a superset of old API.
> > If you want the old API left in place I can just add the wrapper
> > described and only test the newly named function and thus indirectly
> > test the old request_firmware_into_buf.
> Yes this makes sense. But I want to take a bit step back right now and
> think about this a bit more. I'm starting to wonder if this whole sysfs
> stuff should be replaced with a better scalable scheme. Consider all the
> fancy things you can do in userspace with a block device. Offsets are
> already supported, and so much more.

Yes, if normal file operations worked in kernel space all would be good.

> So I'm starting to think that the
> firmware fallback upload sysfs interface is much better suited as a
> really simple block device long term.
> I understand you want your solutions addressed upstream yesterday, but
> this is the *sort of review* on architecture that should have been
> done for the request_firmware_into_buf() long ago. But since you
> probably don't want to wait for a revamp of the interface, a middle
> ground might be in order for now, with the roadmap put in place to
> evaluate scalable alternatives.

Sounds very reasonable.

All I wish to do is request part of file into a pre-allocated memory 
location.

> Either way, we should consider the current bug you ran into for the
> solutions put forward, with the new functionality you are proposing.
>
> The core of the issue you ran into was the duplicate named kobjects,
> which are reflected also on the sysfs hierarchy. The directory name
> created for each firmware request, when duplicate entries exist for
> one device collide. Upon a secondary request for firmware using the
> fallback interface, the kobject/directory already exists.
>
> Its easier to understand this from a directory hierarchy perspective.
> For instance the test driver uses:
>
> /sys/devices/virtual/misc/test_firmware/
>
> The test script for the test_firmware driver uses:
>
> DIR=/sys/devices/virtual/misc/test_firmware/
>
> To load firmware we use a directory underneath this firmware name for
> the file name of the firmware requested, so to load firmware called
> $name on the test script we use:
>
> echo 1 >"$DIR"/"$name"/loading
> cat "$file" >"$DIR"/"$name"/data
> echo 0 >"$DIR"/"$name"/loading
>
> An issue no one has cared for, and also we have not hit yet is that,
> this implies no firmware names can be used which match other sysfs
> attributes exported by a driver. I'm not too concerned for this right
> now, but it is a worthy thing to consider long term under a new
> solution.
>
> So the issue is that the firmware loader will try to create two equally
> named entries underneath the firmware loader directory. Yes we can
> sledge hammer the API to act serially, but this is will just
> just move one problem to another, your secondary call would have to
> wait until the first one not only completes the call, but also
> release_firmware() is called.
>
> I'm looking at using a device name prefix if we do add a new API
> or functionality. This way userspace can expend and knows what
> extra tag to use other than the driver name.
>
>    Luis