[2/7] firmware: add offset to request_firmware_into_buf
diff mbox series

Message ID 20190822192451.5983-3-scott.branden@broadcom.com
State New
Headers show
Series
  • firmware: add partial read support in request_firmware_into_buf
Related show

Commit Message

Scott Branden Aug. 22, 2019, 7:24 p.m. UTC
Add offset to request_firmware_into_buf to allow for portions
of firmware file to be read into a buffer.  Necessary where firmware
needs to be loaded in portions from file in memory constrained systems.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 drivers/base/firmware_loader/firmware.h |  5 +++
 drivers/base/firmware_loader/main.c     | 49 +++++++++++++++++--------
 drivers/soc/qcom/mdt_loader.c           |  7 +++-
 include/linux/firmware.h                |  8 +++-
 lib/test_firmware.c                     |  4 +-
 5 files changed, 53 insertions(+), 20 deletions(-)

Comments

Luis Chamberlain Aug. 22, 2019, 7:47 p.m. UTC | #1
On Thu, Aug 22, 2019 at 12:24:46PM -0700, Scott Branden wrote:
> @@ -923,16 +936,22 @@ EXPORT_SYMBOL_GPL(firmware_request_cache);
>   */
>  int
>  request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
> -			  struct device *device, void *buf, size_t size)
> +			  struct device *device, void *buf, size_t size,
> +			  size_t offset, unsigned int pread_flags)

This implies you having to change the other callers, and while currently
our list of drivers is small, 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()

And thanks for adding a test case!

  Luis
Scott Branden Aug. 22, 2019, 8:07 p.m. UTC | #2
Hi Luis,

On 2019-08-22 12:47 p.m., Luis Chamberlain wrote:
> On Thu, Aug 22, 2019 at 12:24:46PM -0700, Scott Branden wrote:
>> @@ -923,16 +936,22 @@ EXPORT_SYMBOL_GPL(firmware_request_cache);
>>    */
>>   int
>>   request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
>> -			  struct device *device, void *buf, size_t size)
>> +			  struct device *device, void *buf, size_t size,
>> +			  size_t offset, unsigned int pread_flags)
> 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 existing API was never tested until I submitted a test case.

And, the maintainer of that driver wanted

to start utilizing my enhanced API instead of the current API.

As such I think it is very reasonable to update the API right now.

> 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?

>
> And thanks for adding a test case!
>
>    Luis

Regards,

  Scott
Luis Chamberlain Aug. 22, 2019, 9:12 p.m. UTC | #3
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.

> 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.

> > 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.

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.

  Luis
Scott Branden Aug. 22, 2019, 11:30 p.m. UTC | #4
Hi Luis,

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

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

>
>> 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?

>
>>> 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?  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.

>
> 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 te

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.

> sting.
>
>    Luis
Takashi Iwai Aug. 23, 2019, 10:05 a.m. UTC | #5
On Thu, 22 Aug 2019 21:24:46 +0200,
Scott Branden wrote:
> 
> Add offset to request_firmware_into_buf to allow for portions
> of firmware file to be read into a buffer.  Necessary where firmware
> needs to be loaded in portions from file in memory constrained systems.

AFAIU, this won't work with the fallback user helper, right?
Also it won't work for the compressed firmware files as-is.

So this new API usage is for the limited use cases, hence it needs
such checks and returns error/warns if the condition isn't met.

IOW, this can't be a simple extension of request_firmware_into_buf()
to pass a new flag.


thanks,

Takashi
Luis Chamberlain Aug. 23, 2019, 3:47 p.m. UTC | #6
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@linux.ibm.com>

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.

> 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.

> > 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. 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.

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
Scott Branden Aug. 23, 2019, 7:44 p.m. UTC | #7
Hi Takashi,

Thanks for review.  comments below.

On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
> On Thu, 22 Aug 2019 21:24:46 +0200,
> Scott Branden wrote:
>> Add offset to request_firmware_into_buf to allow for portions
>> of firmware file to be read into a buffer.  Necessary where firmware
>> needs to be loaded in portions from file in memory constrained systems.
> AFAIU, this won't work with the fallback user helper, right?
Seems to work fine in the fw_run_tests.sh with fallbacks.
> Also it won't work for the compressed firmware files as-is.
Although unnecessary, seems to work fine in the fw_run_tests.sh with 
"both" and "xzonly" options.
>
> So this new API usage is for the limited use cases, hence it needs
> such checks and returns error/warns if the condition isn't met.
>
> IOW, this can't be a simple extension of request_firmware_into_buf()
> to pass a new flag.
>
>
> thanks,
>
> Takashi
Scott Branden Aug. 23, 2019, 8:16 p.m. UTC | #8
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@linux.ibm.com>
>
> 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
Takashi Iwai Aug. 26, 2019, 3:20 p.m. UTC | #9
On Fri, 23 Aug 2019 21:44:42 +0200,
Scott Branden wrote:
> 
> Hi Takashi,
> 
> Thanks for review.  comments below.
> 
> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
> > On Thu, 22 Aug 2019 21:24:46 +0200,
> > Scott Branden wrote:
> >> Add offset to request_firmware_into_buf to allow for portions
> >> of firmware file to be read into a buffer.  Necessary where firmware
> >> needs to be loaded in portions from file in memory constrained systems.
> > AFAIU, this won't work with the fallback user helper, right?
> Seems to work fine in the fw_run_tests.sh with fallbacks.

But how?  You patch doesn't change anything about the fallback loading
mechanism.  Or, if the expected behavior is to load the whole content
and then copy a part, what's the merit of this API?

> > Also it won't work for the compressed firmware files as-is.
> Although unnecessary, seems to work fine in the fw_run_tests.sh with
> "both" and "xzonly" options.

This looks also suspicious.  Loading a part of the file from the
middle and decompression won't work together, from obvious reasons.

If the test passes, it means that the test itself is more likely
incorrect, I'm afraid.


thanks,

Takashi
Scott Branden Aug. 26, 2019, 3:41 p.m. UTC | #10
HI Takashi,

On 2019-08-26 8:20 a.m., Takashi Iwai wrote:
> On Fri, 23 Aug 2019 21:44:42 +0200,
> Scott Branden wrote:
>> Hi Takashi,
>>
>> Thanks for review.  comments below.
>>
>> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
>>> On Thu, 22 Aug 2019 21:24:46 +0200,
>>> Scott Branden wrote:
>>>> Add offset to request_firmware_into_buf to allow for portions
>>>> of firmware file to be read into a buffer.  Necessary where firmware
>>>> needs to be loaded in portions from file in memory constrained systems.
>>> AFAIU, this won't work with the fallback user helper, right?
>> Seems to work fine in the fw_run_tests.sh with fallbacks.
> But how?  You patch doesn't change anything about the fallback loading
> mechanism.
Correct - I didn't change any of the underlying mechanisms,
so however request_firmware_into_buf worked before it still does.
>   Or, if the expected behavior is to load the whole content
> and then copy a part, what's the merit of this API?
The merit of the API is that the entire file is not copied into a buffer.
In my use case, the buffer is a memory region in PCIe space that isn't
even large enough for the whole file.  So the only way to get the file 
is to read it
in portions.
>
>>> Also it won't work for the compressed firmware files as-is.
>> Although unnecessary, seems to work fine in the fw_run_tests.sh with
>> "both" and "xzonly" options.
> This looks also suspicious.  Loading a part of the file from the
> middle and decompression won't work together, from obvious reasons.
I don't know what the underlying mechanisms are doing right now.
If they decompress the whole file then that is why it's working.
An obvious improvement that could be made later is to only read
a portion of the file before writing it into the buffer in the non-xz case.
>
> If the test passes, it means that the test itself is more likely
> incorrect, I'm afraid.
Then all of the tests for "both" and "xzonly" could be broken.
>
>
> thanks,
>
> Takashi
Regards,
  Scott
Takashi Iwai Aug. 26, 2019, 3:57 p.m. UTC | #11
On Mon, 26 Aug 2019 17:41:40 +0200,
Scott Branden wrote:
> 
> HI Takashi,
> 
> On 2019-08-26 8:20 a.m., Takashi Iwai wrote:
> > On Fri, 23 Aug 2019 21:44:42 +0200,
> > Scott Branden wrote:
> >> Hi Takashi,
> >>
> >> Thanks for review.  comments below.
> >>
> >> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
> >>> On Thu, 22 Aug 2019 21:24:46 +0200,
> >>> Scott Branden wrote:
> >>>> Add offset to request_firmware_into_buf to allow for portions
> >>>> of firmware file to be read into a buffer.  Necessary where firmware
> >>>> needs to be loaded in portions from file in memory constrained systems.
> >>> AFAIU, this won't work with the fallback user helper, right?
> >> Seems to work fine in the fw_run_tests.sh with fallbacks.
> > But how?  You patch doesn't change anything about the fallback loading
> > mechanism.
> Correct - I didn't change any of the underlying mechanisms,
> so however request_firmware_into_buf worked before it still does.

But how?  That's the question.

If I understand correctly, essentially your patch changes the call of
kernel_read_file_from_path() with additional offset and partial size
parameters.  i.e. the partial read behavior itself purely relies on
the kernel_read_file_from_path().
And, if the file isn't read via this function, the f/w loader falls
back to the UMH.  Since fallback.c has no idea about the partial read,
it shall return the full content of the file.  Then this must
contradict against the expected result, no?

> >   Or, if the expected behavior is to load the whole content
> > and then copy a part, what's the merit of this API?
> The merit of the API is that the entire file is not copied into a buffer.
> In my use case, the buffer is a memory region in PCIe space that isn't
> even large enough for the whole file.  So the only way to get the file
> is to read it
> in portions.

But you read not in portions but the whole content, in the case of
fallback mode...

> >>> Also it won't work for the compressed firmware files as-is.
> >> Although unnecessary, seems to work fine in the fw_run_tests.sh with
> >> "both" and "xzonly" options.
> > This looks also suspicious.  Loading a part of the file from the
> > middle and decompression won't work together, from obvious reasons.
> I don't know what the underlying mechanisms are doing right now.
> If they decompress the whole file then that is why it's working.

No, it shouldn't be a complete read.  As already mentioned, the patch
changes only the call pattern of kernel_read_file_from_path().  The
decompression is done after that, so it must be applied to the
partially read content which cannot be decompressed properly.

> An obvious improvement that could be made later is to only read
> a portion of the file before writing it into the buffer in the non-xz case.
>
> > If the test passes, it means that the test itself is more likely
> > incorrect, I'm afraid.
> Then all of the tests for "both" and "xzonly" could be broken.

I suspect that the fallback test is also broken.


thanks,

Takashi
Takashi Iwai Aug. 26, 2019, 5:12 p.m. UTC | #12
On Mon, 26 Aug 2019 17:41:40 +0200,
Scott Branden wrote:
> 
> HI Takashi,
> 
> On 2019-08-26 8:20 a.m., Takashi Iwai wrote:
> > On Fri, 23 Aug 2019 21:44:42 +0200,
> > Scott Branden wrote:
> >> Hi Takashi,
> >>
> >> Thanks for review.  comments below.
> >>
> >> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
> >>> On Thu, 22 Aug 2019 21:24:46 +0200,
> >>> Scott Branden wrote:
> >>>> Add offset to request_firmware_into_buf to allow for portions
> >>>> of firmware file to be read into a buffer.  Necessary where firmware
> >>>> needs to be loaded in portions from file in memory constrained systems.
> >>> AFAIU, this won't work with the fallback user helper, right?
> >> Seems to work fine in the fw_run_tests.sh with fallbacks.
> > But how?  You patch doesn't change anything about the fallback loading
> > mechanism.
> Correct - I didn't change any of the underlying mechanisms,
> so however request_firmware_into_buf worked before it still does.
> >   Or, if the expected behavior is to load the whole content
> > and then copy a part, what's the merit of this API?
> The merit of the API is that the entire file is not copied into a buffer.
> In my use case, the buffer is a memory region in PCIe space that isn't
> even large enough for the whole file.  So the only way to get the file
> is to read it
> in portions.

BTW: does the use case above mean that the firmware API directly
writes onto the given PCI iomem region?  If so, I'm not sure whether
it would work as expected on all architectures.  There must be a
reason of the presence of iomem-related API like memcpy_toio()...


thanks,

Takashi
Scott Branden Aug. 26, 2019, 5:24 p.m. UTC | #13
Hi Takashi,

On 2019-08-26 10:12 a.m., Takashi Iwai wrote:
> On Mon, 26 Aug 2019 17:41:40 +0200,
> Scott Branden wrote:
>> HI Takashi,
>>
>> On 2019-08-26 8:20 a.m., Takashi Iwai wrote:
>>> On Fri, 23 Aug 2019 21:44:42 +0200,
>>> Scott Branden wrote:
>>>> Hi Takashi,
>>>>
>>>> Thanks for review.  comments below.
>>>>
>>>> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
>>>>> On Thu, 22 Aug 2019 21:24:46 +0200,
>>>>> Scott Branden wrote:
>>>>>> Add offset to request_firmware_into_buf to allow for portions
>>>>>> of firmware file to be read into a buffer.  Necessary where firmware
>>>>>> needs to be loaded in portions from file in memory constrained systems.
>>>>> AFAIU, this won't work with the fallback user helper, right?
>>>> Seems to work fine in the fw_run_tests.sh with fallbacks.
>>> But how?  You patch doesn't change anything about the fallback loading
>>> mechanism.
>> Correct - I didn't change any of the underlying mechanisms,
>> so however request_firmware_into_buf worked before it still does.
>>>    Or, if the expected behavior is to load the whole content
>>> and then copy a part, what's the merit of this API?
>> The merit of the API is that the entire file is not copied into a buffer.
>> In my use case, the buffer is a memory region in PCIe space that isn't
>> even large enough for the whole file.  So the only way to get the file
>> is to read it
>> in portions.
> BTW: does the use case above mean that the firmware API directly
> writes onto the given PCI iomem region?  If so, I'm not sure whether
> it would work as expected on all architectures.  There must be a
> reason of the presence of iomem-related API like memcpy_toio()...
Yes, we access the PCI region directly in the driver and thus also 
through request_firmware_into_buf.
I will admit I am not familiar with every subtlety of PCI accesses. Any 
comments to the Valkyrie driver in this patch series are appreciated.
But not all drivers need to work on all architectures. I can add a 
depends on x86 64bit architectures to the driver to limit it to such.
>
>
> thanks,
>
> Takashi
Takashi Iwai Aug. 27, 2019, 10:40 a.m. UTC | #14
On Mon, 26 Aug 2019 19:24:22 +0200,
Scott Branden wrote:
> 
> Hi Takashi,
> 
> On 2019-08-26 10:12 a.m., Takashi Iwai wrote:
> > On Mon, 26 Aug 2019 17:41:40 +0200,
> > Scott Branden wrote:
> >> HI Takashi,
> >>
> >> On 2019-08-26 8:20 a.m., Takashi Iwai wrote:
> >>> On Fri, 23 Aug 2019 21:44:42 +0200,
> >>> Scott Branden wrote:
> >>>> Hi Takashi,
> >>>>
> >>>> Thanks for review.  comments below.
> >>>>
> >>>> On 2019-08-23 3:05 a.m., Takashi Iwai wrote:
> >>>>> On Thu, 22 Aug 2019 21:24:46 +0200,
> >>>>> Scott Branden wrote:
> >>>>>> Add offset to request_firmware_into_buf to allow for portions
> >>>>>> of firmware file to be read into a buffer.  Necessary where firmware
> >>>>>> needs to be loaded in portions from file in memory constrained systems.
> >>>>> AFAIU, this won't work with the fallback user helper, right?
> >>>> Seems to work fine in the fw_run_tests.sh with fallbacks.
> >>> But how?  You patch doesn't change anything about the fallback loading
> >>> mechanism.
> >> Correct - I didn't change any of the underlying mechanisms,
> >> so however request_firmware_into_buf worked before it still does.
> >>>    Or, if the expected behavior is to load the whole content
> >>> and then copy a part, what's the merit of this API?
> >> The merit of the API is that the entire file is not copied into a buffer.
> >> In my use case, the buffer is a memory region in PCIe space that isn't
> >> even large enough for the whole file.  So the only way to get the file
> >> is to read it
> >> in portions.
> > BTW: does the use case above mean that the firmware API directly
> > writes onto the given PCI iomem region?  If so, I'm not sure whether
> > it would work as expected on all architectures.  There must be a
> > reason of the presence of iomem-related API like memcpy_toio()...
> Yes, we access the PCI region directly in the driver and thus also
> through request_firmware_into_buf.

Then you really need to access via the standard APIs for iomem.
The normal memory copy would work only on some architectures like
x86.

> I will admit I am not familiar with every subtlety of PCI
> accesses. Any comments to the Valkyrie driver in this patch series are
> appreciated.
> But not all drivers need to work on all architectures. I can add a
> depends on x86 64bit architectures to the driver to limit it to such.

But it's an individual board on PCIe, and should work no matter which
architecture is?  Or is this really exclusive to x86?


thanks,

Takashi
Luis Chamberlain Oct. 11, 2019, 1:31 p.m. UTC | #15
On Tue, Aug 27, 2019 at 12:40:02PM +0200, Takashi Iwai wrote:
> On Mon, 26 Aug 2019 19:24:22 +0200,
> Scott Branden wrote:
> > 
> > I will admit I am not familiar with every subtlety of PCI
> > accesses. Any comments to the Valkyrie driver in this patch series are
> > appreciated.
> > But not all drivers need to work on all architectures. I can add a
> > depends on x86 64bit architectures to the driver to limit it to such.
> 
> But it's an individual board on PCIe, and should work no matter which
> architecture is?  Or is this really exclusive to x86?

Poke Scott.

  Luis

Patch
diff mbox series

diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 7ecd590e67fe..4b8997e84ceb 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -29,6 +29,8 @@ 
  *	firmware caching mechanism.
  * @FW_OPT_NOFALLBACK: Disable the fallback mechanism. Takes precedence over
  *	&FW_OPT_UEVENT and &FW_OPT_USERHELPER.
+ * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
+ *	entire file.
  */
 enum fw_opt {
 	FW_OPT_UEVENT =         BIT(0),
@@ -37,6 +39,7 @@  enum fw_opt {
 	FW_OPT_NO_WARN =        BIT(3),
 	FW_OPT_NOCACHE =        BIT(4),
 	FW_OPT_NOFALLBACK =     BIT(5),
+	FW_OPT_PARTIAL =        BIT(6),
 };
 
 enum fw_status {
@@ -64,6 +67,8 @@  struct fw_priv {
 	void *data;
 	size_t size;
 	size_t allocated_size;
+	size_t offset;
+	unsigned int flags;
 #ifdef CONFIG_FW_LOADER_PAGED_BUF
 	bool is_paged_buf;
 	struct page **pages;
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index bf44c79beae9..0e37268f1e47 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -167,7 +167,8 @@  static int fw_cache_piggyback_on_request(const char *name);
 
 static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 					  struct firmware_cache *fwc,
-					  void *dbuf, size_t size)
+					  void *dbuf, size_t size,
+					  size_t offset, unsigned int flags)
 {
 	struct fw_priv *fw_priv;
 
@@ -185,6 +186,8 @@  static struct fw_priv *__allocate_fw_priv(const char *fw_name,
 	fw_priv->fwc = fwc;
 	fw_priv->data = dbuf;
 	fw_priv->allocated_size = size;
+	fw_priv->offset = offset;
+	fw_priv->flags = flags;
 	fw_state_init(fw_priv);
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 	INIT_LIST_HEAD(&fw_priv->pending_list);
@@ -210,9 +213,11 @@  static struct fw_priv *__lookup_fw_priv(const char *fw_name)
 static int alloc_lookup_fw_priv(const char *fw_name,
 				struct firmware_cache *fwc,
 				struct fw_priv **fw_priv, void *dbuf,
-				size_t size, enum fw_opt opt_flags)
+				size_t size, enum fw_opt opt_flags,
+				size_t offset)
 {
 	struct fw_priv *tmp;
+	unsigned int pread_flags;
 
 	spin_lock(&fwc->lock);
 	if (!(opt_flags & FW_OPT_NOCACHE)) {
@@ -226,7 +231,12 @@  static int alloc_lookup_fw_priv(const char *fw_name,
 		}
 	}
 
-	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
+	if (opt_flags & FW_OPT_PARTIAL)
+		pread_flags = KERNEL_PREAD_FLAG_PART;
+	else
+		pread_flags = KERNEL_PREAD_FLAG_WHOLE;
+
+	tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, pread_flags);
 	if (tmp) {
 		INIT_LIST_HEAD(&tmp->list);
 		if (!(opt_flags & FW_OPT_NOCACHE))
@@ -493,8 +503,9 @@  fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
 		}
 
 		fw_priv->size = 0;
-		rc = kernel_read_file_from_path(path, &buffer, &size,
-						msize, id);
+		rc = kernel_pread_file_from_path(path, &buffer, &size,
+						 fw_priv->offset, msize,
+						 fw_priv->flags, id);
 		if (rc) {
 			if (rc != -ENOENT)
 				dev_warn(device, "loading %s failed with error %d\n",
@@ -684,7 +695,7 @@  int assign_fw(struct firmware *fw, struct device *device,
 static int
 _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 			  struct device *device, void *dbuf, size_t size,
-			  enum fw_opt opt_flags)
+			  enum fw_opt opt_flags, size_t offset)
 {
 	struct firmware *firmware;
 	struct fw_priv *fw_priv;
@@ -703,7 +714,7 @@  _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	}
 
 	ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
-				  opt_flags);
+				  opt_flags, offset);
 
 	/*
 	 * bind with 'priv' now to avoid warning in failure path
@@ -750,7 +761,7 @@  static void fw_abort_batch_reqs(struct firmware *fw)
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
 		  struct device *device, void *buf, size_t size,
-		  enum fw_opt opt_flags)
+		  enum fw_opt opt_flags, size_t offset)
 {
 	struct firmware *fw = NULL;
 	int ret;
@@ -764,7 +775,7 @@  _request_firmware(const struct firmware **firmware_p, const char *name,
 	}
 
 	ret = _request_firmware_prepare(&fw, name, device, buf, size,
-					opt_flags);
+					opt_flags, offset);
 	if (ret <= 0) /* error or already assigned */
 		goto out;
 
@@ -824,7 +835,7 @@  request_firmware(const struct firmware **firmware_p, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
-				FW_OPT_UEVENT);
+				FW_OPT_UEVENT, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -851,7 +862,7 @@  int firmware_request_nowarn(const struct firmware **firmware, const char *name,
 	/* Need to pin this module until return */
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware, name, device, NULL, 0,
-				FW_OPT_UEVENT | FW_OPT_NO_WARN);
+				FW_OPT_UEVENT | FW_OPT_NO_WARN, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -876,7 +887,7 @@  int request_firmware_direct(const struct firmware **firmware_p,
 	__module_get(THIS_MODULE);
 	ret = _request_firmware(firmware_p, name, device, NULL, 0,
 				FW_OPT_UEVENT | FW_OPT_NO_WARN |
-				FW_OPT_NOFALLBACK);
+				FW_OPT_NOFALLBACK, 0);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -913,6 +924,8 @@  EXPORT_SYMBOL_GPL(firmware_request_cache);
  * @device: device for which firmware is being loaded and DMA region allocated
  * @buf: address of buffer to load firmware into
  * @size: size of buffer
+ * @offset: offset into file to read
+ * @pread_flags: KERNEL_PREAD_FLAG_PART to allow partial file read
  *
  * This function works pretty much like request_firmware(), but it doesn't
  * allocate a buffer to hold the firmware data. Instead, the firmware
@@ -923,16 +936,22 @@  EXPORT_SYMBOL_GPL(firmware_request_cache);
  */
 int
 request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
-			  struct device *device, void *buf, size_t size)
+			  struct device *device, void *buf, size_t size,
+			  size_t offset, unsigned int pread_flags)
 {
 	int ret;
+	enum fw_opt opt_flags;
 
 	if (fw_cache_is_setup(device, name))
 		return -EOPNOTSUPP;
 
 	__module_get(THIS_MODULE);
+	opt_flags = FW_OPT_UEVENT | FW_OPT_NOCACHE;
+	if (pread_flags & KERNEL_PREAD_FLAG_PART)
+		opt_flags |= FW_OPT_PARTIAL;
+
 	ret = _request_firmware(firmware_p, name, device, buf, size,
-				FW_OPT_UEVENT | FW_OPT_NOCACHE);
+				opt_flags, offset);
 	module_put(THIS_MODULE);
 	return ret;
 }
@@ -971,7 +990,7 @@  static void request_firmware_work_func(struct work_struct *work)
 	fw_work = container_of(work, struct firmware_work, work);
 
 	_request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
-			  fw_work->opt_flags);
+			  fw_work->opt_flags, 0);
 	fw_work->cont(fw, fw_work->context);
 	put_device(fw_work->device); /* taken in request_firmware_nowait() */
 
diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
index 24cd193dec55..00f3359f4f61 100644
--- a/drivers/soc/qcom/mdt_loader.c
+++ b/drivers/soc/qcom/mdt_loader.c
@@ -246,8 +246,11 @@  static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
 		} else if (phdr->p_filesz) {
 			/* Firmware not large enough, load split-out segments */
 			sprintf(fw_name + fw_name_len - 3, "b%02d", i);
-			ret = request_firmware_into_buf(&seg_fw, fw_name, dev,
-							ptr, phdr->p_filesz);
+			ret = request_firmware_into_buf
+						(&seg_fw, fw_name, dev,
+						 ptr, phdr->p_filesz,
+						 0,
+						 KERNEL_PREAD_FLAG_WHOLE);
 			if (ret) {
 				dev_err(dev, "failed to load %s\n", fw_name);
 				break;
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 2dd566c91d44..c81162a8d709 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -4,6 +4,7 @@ 
 
 #include <linux/types.h>
 #include <linux/compiler.h>
+#include <linux/fs.h>
 #include <linux/gfp.h>
 
 #define FW_ACTION_NOHOTPLUG 0
@@ -51,7 +52,9 @@  int request_firmware_nowait(
 int request_firmware_direct(const struct firmware **fw, const char *name,
 			    struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size);
+			      const char *name, struct device *device,
+			      void *buf, size_t size,
+			      size_t offset, unsigned int pread_flags);
 
 void release_firmware(const struct firmware *fw);
 #else
@@ -89,7 +92,8 @@  static inline int request_firmware_direct(const struct firmware **fw,
 }
 
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
-	const char *name, struct device *device, void *buf, size_t size)
+	const char *name, struct device *device, void *buf, size_t size,
+	size_t offset, unsigned int pread_flags);
 {
 	return -EINVAL;
 }
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 251213c872b5..7d1d97fa9a23 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -622,7 +622,9 @@  static int test_fw_run_batch_request(void *data)
 						    req->name,
 						    req->dev,
 						    test_buf,
-						    TEST_FIRMWARE_BUF_SIZE);
+						    TEST_FIRMWARE_BUF_SIZE,
+						    0,
+						    KERNEL_PREAD_FLAG_WHOLE);
 		if (!req->fw)
 			kfree(test_buf);
 	} else {