Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status

From: Daniel Wagner
Date: Tue Oct 18 2016 - 09:31:01 EST

On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
On Fri, Oct 07, 2016 at 01:41:21PM +0200, Daniel Wagner wrote:
On 10/05/2016 10:27 PM, Luis R. Rodriguez wrote:
On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote:
On 09/09/2016 02:12 PM, Daniel Wagner wrote:
The firmware user helper code tracks the current state of the loading
process via unsigned long status and a completion in struct
firmware_buf. We only need this for the usermode helper as such we can
encapsulate all this data into its own data structure.

I don't think we are able to move the completion code into a
CONFIG_FW_LOADER_HELPER section. The direct loading path uses
completion as well.


If you look at the current code (not these patches) you have dependency via
the firmware_buf for two concurrent _request_firmware() calls:

1nd request (waker context)

fw_lookup_and_allocate_buf() # no pendending request
# returns 0 -> load firmware

"no pending request" is an invalid association with what fw_lookup_and_allocate_buf()
does, its also why I have asked for this to be renamed, it looks for the firmware
in the fw cache, if it finds it it returns 1. Otherwise it creates a new buf
entry and adds it to the fw cache, and returns 0.

I used 'no pending request' for what you describe below with first call. So yes, either the buffer is allocated for the given name or it's missing. It doesn't tell anything about the load status of the firmware.


2nd request (waiter context)

fw_lookup_allocate_buf() # finds previously allocated buf
# returns 1 -> wait for loading

No, that's wait_for_completion_interruptible() not

I confused that one from _request_firmware_load().

Also note that we only call sync_cached_firmware_buf()
*iff* fw_lookup_and_allocate_buf() returned the 1 -- I mentioned
when this happens above. That happens only if we already had the entry on
the fw cache. As it stands -- concurrent calls against the same fw name
could cause a clash here, as such, the wait_for_completion_interruptible()
is indeed still needed.

Further optimizations can be considered later but for indeed, agreed
that completion is needed even for the direct fw load case. The timeout
though, I don't see a reason for it.

So I think I found the source of the confusion about fw_umh_wait_timeout(). When providing a timeout value of 0 you get the wait_for_completion_interruptible() version.

+#define fw_umh_wait_timeout(fw_st, long) 0
+#define fw_umh_done(fw_st)
+#define fw_umh_is_done(fw_st) true
+#define fw_umh_is_aborted(fw_st) false

We still need the implementation for fw_umh_wait_timeout() and
fw_umh_start(), fw_umh_done() etc.


See above.

Sure, but note how the timeout is not used.

See above.

@@ -309,8 +373,7 @@ static void fw_finish_direct_load(struct device *device,
struct firmware_buf *buf)
- set_bit(FW_STATUS_DONE, &buf->status);
- complete_all(&buf->completion);
+ fw_umh_done(&buf->fw_umh);

Here we signal that we have loaded the firmware

The struct firmware_buf is only used for the sysfs stuff no?

I don't know, I was looking at the code in firmware_class.c not any users.
Why is that important?

Sorry I meant struct firmware_priv is used by sysfs stuff only, the sysfs stuff
is only used for the FW UMH.

Yes, even 'struct firmware_priv' is only available when CONFIG_FW_LOADER_USER_HELPER. Though fw_finish_direct_load() is used in the !UHM section. I think I still miss your point here.

/* wait until the shared firmware_buf becomes ready (or error) */
static int sync_cached_firmware_buf(struct firmware_buf *buf)
int ret = 0;

- while (!test_bit(FW_STATUS_DONE, &buf->status)) {
- if (is_fw_load_aborted(buf)) {
+ while (!fw_umh_is_done(&buf->fw_umh)) {
+ if (fw_umh_is_aborted(&buf->fw_umh)) {
ret = -ENOENT;
- ret = wait_for_completion_interruptible(&buf->completion);
+ ret = fw_umh_wait_timeout(&buf->fw_umh, 0);

and here we here we wait for it.


As I tried to explain above the buffering code is depending on completion.

OK sure agreed. The timeout, no though, unless I missed something?

I don't think so. The only thing is the value of the fw_uhm_wait_timeout().