archive mirror
 help / color / mirror / Atom feed
From: Scott Branden <>
To: Luis Chamberlain <>
Cc: Greg Kroah-Hartman <>,
	Mimi Zohar <>,
	David Brown <>,
	Alexander Viro <>,
	Shuah Khan <>,,
	Shuah Khan <>,
	Arnd Bergmann <>,
	"Rafael J . Wysocki" <>,,,,
	BCM Kernel Feedback <>,
	Olof Johansson <>,
	Andrew Morton <>,
	Dan Carpenter <>,
	Colin Ian King <>,
	Kees Cook <>, Takashi Iwai <>,
Subject: Re: [PATCH 2/7] firmware: add offset to request_firmware_into_buf
Date: Fri, 23 Aug 2019 13:16:32 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

> 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

  reply	other threads:[~2019-08-23 20:16 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-22 19:24 [PATCH 0/7] firmware: add partial read support in request_firmware_into_buf Scott Branden
2019-08-22 19:24 ` [PATCH 1/7] fs: introduce kernel_pread_file* support Scott Branden
2019-08-23 12:29   ` Takashi Iwai
2019-08-23 19:55     ` Scott Branden
2019-08-23 21:29       ` Luis Chamberlain
2019-08-22 19:24 ` [PATCH 2/7] firmware: add offset to request_firmware_into_buf Scott Branden
2019-08-22 19:47   ` Luis Chamberlain
2019-08-22 20:07     ` Scott Branden
2019-08-22 21:12       ` Luis Chamberlain
2019-08-22 23:30         ` Scott Branden
2019-08-23 15:47           ` Luis Chamberlain
2019-08-23 20:16             ` Scott Branden [this message]
2019-08-23 10:05   ` Takashi Iwai
2019-08-23 19:44     ` Scott Branden
2019-08-26 15:20       ` Takashi Iwai
2019-08-26 15:41         ` Scott Branden
2019-08-26 15:57           ` Takashi Iwai
2019-08-26 17:12           ` Takashi Iwai
2019-08-26 17:24             ` Scott Branden
2019-08-27 10:40               ` Takashi Iwai
2019-10-11 13:31                 ` Luis Chamberlain
2020-02-21  0:11                   ` Scott Branden
2020-02-21  8:44                     ` Arnd Bergmann
2020-02-21 18:23                       ` Scott Branden
2020-02-21 23:37                       ` Scott Branden
2020-02-22  8:06                         ` Arnd Bergmann
2019-08-22 19:24 ` [PATCH 3/7] test_firmware: add partial read support for request_firmware_into_buf Scott Branden
2019-08-22 19:24 ` [PATCH 4/7] selftests: firmware: Test partial file reads of request_firmware_into_buf Scott Branden
2019-08-22 19:24 ` [PATCH 5/7] bcm-vk: add bcm_vk UAPI Scott Branden
2019-08-27 13:54   ` Arnd Bergmann
2019-08-27 14:49   ` Kieran Bingham
2019-10-08 15:59     ` Olof Johansson
2019-08-22 19:24 ` [PATCH 6/7] misc: bcm-vk: add Broadcom Valkyrie driver Scott Branden
2019-08-27 14:14   ` Arnd Bergmann
2019-08-27 15:25     ` Nicolas Dufresne
2019-08-22 19:24 ` [PATCH 7/7] MAINTAINERS: bcm-vk: Add maintainer for Broadcom Valkyrie Driver Scott Branden

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).