Re: [PATCH v4 2/4] firmware: encapsulate firmware loading status

From: Daniel Wagner
Date: Thu Sep 08 2016 - 04:05:42 EST


On 09/08/2016 03:39 AM, Luis R. Rodriguez wrote:
On Wed, Sep 07, 2016 at 10:45:06AM +0200, Daniel Wagner wrote:
From: Daniel Wagner <daniel.wagner@xxxxxxxxxxxx>
timeout = MAX_JIFFY_OFFSET;
}

- retval = wait_for_completion_interruptible_timeout(&buf->completion,
- timeout);
+ retval = fw_status_wait_timeout(&buf->fw_st, timeout);

Note this is one of the users for fw_status_wait_timeout(). This makes sense
for this fw_status_wait_timeout, as its umh related.

if (retval == -ERESTARTSYS || !retval) {
mutex_lock(&fw_lock);
fw_load_abort(fw_priv);
@@ -965,7 +1022,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
retval = 0;
}

- if (is_fw_load_aborted(buf))
+ if (fw_status_is_aborted(&buf->fw_st))
retval = -EAGAIN;
else if (buf->is_paged_buf && !buf->data)
retval = -ENOMEM;
@@ -1040,29 +1097,25 @@ fw_load_from_user_helper(struct firmware *firmware, const char *name,
return -ENOENT;
}

-/* No abort during direct loading */
-#define is_fw_load_aborted(buf) false
-
#ifdef CONFIG_PM_SLEEP
static inline void kill_requests_without_uevent(void) { }
#endif

#endif /* CONFIG_FW_LOADER_USER_HELPER */

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

mutex_lock(&fw_lock);
- while (!test_bit(FW_STATUS_DONE, &buf->status)) {
- if (is_fw_load_aborted(buf)) {
+ while (!fw_status_is_done(&buf->fw_st)) {
+ if (fw_status_is_aborted(&buf->fw_st)) {
ret = -ENOENT;
break;
}
mutex_unlock(&fw_lock);
- ret = wait_for_completion_interruptible(&buf->completion);
+ ret = fw_status_wait_timeout(&buf->fw_st, 0);

Now this one -- I am not sure of. That it, it is not clear why we would
need fw_status_wait_timeout() here, and the code history does not reveal
that either. Obviously this is a no-op for for non UMH kenrels *but*
even for UMH -- why do we need it?


This while loop was originally a goto loop:

1f2b79599ee8 ("firmware loader: always let firmware_buf own the pages buffer")

I don't think the code doesn't do what it was indented to do. The reason is that calling complete_all() the wait_for_completion() will not block again until it has 'eaten up' the done counter. That is around UMAX/2 loops. So there is an reinit_completion() missing in this case.

Before the above commit, the completion was used to wait inside
_request_firmware_load() till the UML had done its job. The answer for your question is probably in 1f2b79599ee8 ("firmware loader: always let firmware_buf own the pages buffer").

/me starts reading.

cheers,
daniel