| Message ID | 20190822192451.5983-3-scott.branden@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: add partial read support in request_firmware_into_buf | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
On 2019-10-11 6:31 a.m., Luis Chamberlain wrote: > 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 Yes, this is exclusive to x86. In particular, 64-bit x86 server class machines with PCIe gen3 support. There is no reason for these PCIe boards to run in other lower end machines or architectures.
On Fri, Feb 21, 2020 at 1:11 AM Scott Branden <scott.branden@broadcom.com> wrote: > On 2019-10-11 6:31 a.m., Luis Chamberlain wrote: > > 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. > > Yes, this is exclusive to x86. > In particular, 64-bit x86 server class machines with PCIe gen3 support. > There is no reason for these PCIe boards to run in other lower end > machines or architectures. It doesn't really matter that much what you expect your customers to do with your product, or what works a particular machine today, drivers should generally be written in a portable manner anyway and use the documented APIs. memcpy() into an __iomem pointer is not portable and while it probably works on any x86 machine today, please just don't do it. If you use 'sparse' to check your code, that would normally result in an address space warning, unless you add __force and a long comment explaining why you cannot just use memcpy_to_io() instead. At that point, you are already better off usingn memcpy_to_io() ;-) Arnd
On 2020-02-21 12:44 a.m., Arnd Bergmann wrote: > On Fri, Feb 21, 2020 at 1:11 AM Scott Branden > <scott.branden@broadcom.com> wrote: >> On 2019-10-11 6:31 a.m., Luis Chamberlain wrote: >>> 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. >> Yes, this is exclusive to x86. >> In particular, 64-bit x86 server class machines with PCIe gen3 support. >> There is no reason for these PCIe boards to run in other lower end >> machines or architectures. > It doesn't really matter that much what you expect your customers to > do with your product, or what works a particular machine today, drivers > should generally be written in a portable manner anyway and use > the documented APIs. memcpy() into an __iomem pointer is not > portable and while it probably works on any x86 machine today, please > just don't do it. If you use 'sparse' to check your code, that would normally > result in an address space warning, unless you add __force and a > long comment explaining why you cannot just use memcpy_to_io() > instead. At that point, you are already better off usingn memcpy_to_io() ;-) We don't want to allocate to intermediate memory and do another memcpy just to write to pcie. I will have to look into the linux request_firmware_info_buf code and detect whether the buf being request to is in kernel or io memory and perform the operation there. Hopefully such is possible. > > Arnd
Hi Arnd, On 2020-02-21 12:44 a.m., Arnd Bergmann wrote: > On Fri, Feb 21, 2020 at 1:11 AM Scott Branden > <scott.branden@broadcom.com> wrote: >> On 2019-10-11 6:31 a.m., Luis Chamberlain wrote: >>> 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. >> Yes, this is exclusive to x86. >> In particular, 64-bit x86 server class machines with PCIe gen3 support. >> There is no reason for these PCIe boards to run in other lower end >> machines or architectures. > It doesn't really matter that much what you expect your customers to > do with your product, or what works a particular machine today, drivers > should generally be written in a portable manner anyway and use > the documented APIs. memcpy() into an __iomem pointer is not > portable and while it probably works on any x86 machine today, please > just don't do it. If you use 'sparse' to check your code, that would normally > result in an address space warning, unless you add __force and a > long comment explaining why you cannot just use memcpy_to_io() > instead. At that point, you are already better off usingn memcpy_to_io() ;-) > > Arnd I am a not performing a memcpy at all right now. I am calling a request_firmware_into_buf call and do not need to make a copy. This function eventually calls kernel_read_file, which then makes at indirect call in __vfs_read to perform the read to memory. From there I am lost as to what operation happens to achieve this. The read function would need to detect the buf is in io space and perform the necessary operation. Anyone with any knowledge on how to make this read to io space would be appreciated? ssize_t __vfs_read(struct file *file, char __user *buf, size_t count, loff_t *pos) { if (file->f_op->read) return file->f_op->read(file, buf, count, pos); else if (file->f_op->read_iter) return new_sync_read(file, buf, count, pos); else return -EINVAL; } ssize_t kernel_read(struct file *file, void *buf, size_t count, loff_t *pos) { mm_segment_t old_fs; ssize_t result; old_fs = get_fs(); set_fs(KERNEL_DS); /* The cast to a user pointer is valid due to the set_fs() */ result = vfs_read(file, (void __user *)buf, count, pos); set_fs(old_fs); return result; }
On Sat, Feb 22, 2020 at 12:37 AM Scott Branden <scott.branden@broadcom.com> wrote: > On 2020-02-21 12:44 a.m., Arnd Bergmann wrote: > > On Fri, Feb 21, 2020 at 1:11 AM Scott Branden > > <scott.branden@broadcom.com> wrote: > >> On 2019-10-11 6:31 a.m., Luis Chamberlain wrote: > >>> 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. > >> Yes, this is exclusive to x86. > >> In particular, 64-bit x86 server class machines with PCIe gen3 support. > >> There is no reason for these PCIe boards to run in other lower end > >> machines or architectures. > > It doesn't really matter that much what you expect your customers to > > do with your product, or what works a particular machine today, drivers > > should generally be written in a portable manner anyway and use > > the documented APIs. memcpy() into an __iomem pointer is not > > portable and while it probably works on any x86 machine today, please > > just don't do it. If you use 'sparse' to check your code, that would normally > > result in an address space warning, unless you add __force and a > > long comment explaining why you cannot just use memcpy_to_io() > > instead. At that point, you are already better off usingn memcpy_to_io() ;-) > > > > Arnd > I am a not performing a memcpy at all right now. > I am calling a request_firmware_into_buf call and do not need to make a > copy. > This function eventually calls kernel_read_file, which then makes at > indirect call in __vfs_read to perform the read to memory. Well, that comes down to a memcpy() in the end, even if you don't spell it like that in your driver. It may be a copy_from_user(), but clearly not a memcpy_to_io(). > From there I am lost as to what operation happens to achieve this. > The read function would need to detect the buf is in io space and > perform the necessary operation. > Anyone with any knowledge on how to make this read to io space would be > appreciated? I don't think modifying the common code is helpful in this case: any access to PCI MMIO space is inevitably going to be slow, so an extra memcpy() in your driver is not going to cause any noticeable overhead, but the generic functions are meant to be fast for the normal use case and not gain any other features. Arnd
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 {
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(-)