Hi Luis,
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 complection 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.
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+
+enum {
+ FW_UMH_UNKNOWN,
+ FW_UMH_LOADING,
+ FW_UMH_DONE,
+ FW_UMH_ABORTED,
+};
The direct loading path just uses two states, LOADING and DONE. ABORTED
is not used.
+struct fw_umh {
+ struct completion completion;
+ unsigned long status;
+};
+
+static void fw_umh_init(struct fw_umh *fw_umh)
+{
+ init_completion(&fw_umh->completion);
+ fw_umh->status = FW_UMH_UNKNOWN;
+}
+
+static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
+{
+ return test_bit(status, &fw_umh->status);
+}
+
+static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
+{
+ int ret;
+
+ ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
+ timeout);
+ if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
+ return -ENOENT;
+
+ return ret;
+}
+
+static void __fw_umh_set(struct fw_umh *fw_umh,
+ unsigned long status)
+{
+ set_bit(status, &fw_umh->status);
+
+ if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
+ clear_bit(FW_UMH_LOADING, &fw_umh->status);
+ complete_all(&fw_umh->completion);
+ }
+}
+
+#define fw_umh_start(fw_umh) \
+ __fw_umh_set(fw_umh, FW_UMH_LOADING)
+#define fw_umh_done(fw_umh) \
+ __fw_umh_set(fw_umh, FW_UMH_DONE)
+#define fw_umh_aborted(fw_umh) \
+ __fw_umh_set(fw_umh, FW_UMH_ABORTED)
+#define fw_umh_is_loading(fw_umh) \
+ __fw_umh_check(fw_umh, FW_UMH_LOADING)
+#define fw_umh_is_done(fw_umh) \
+ __fw_umh_check(fw_umh, FW_UMH_DONE)
+#define fw_umh_is_aborted(fw_umh) \
+ __fw_umh_check(fw_umh, FW_UMH_ABORTED)
+
+#else /* CONFIG_FW_LOADER_USER_HELPER */
+
+#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. fw_umh_is_aborted() is not needed.
@@ -309,8 +373,7 @@ static void fw_finish_direct_load(struct device *device,
struct firmware_buf *buf)
{
mutex_lock(&fw_lock);
- set_bit(FW_STATUS_DONE, &buf->status);
- complete_all(&buf->completion);
+ fw_umh_done(&buf->fw_umh);
mutex_unlock(&fw_lock);
}
Here we signal that we have loaded the firmware
/* 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_umh_is_done(&buf->fw_umh)) {
+ if (fw_umh_is_aborted(&buf->fw_umh)) {
ret = -ENOENT;
break;
}
mutex_unlock(&fw_lock);
- ret = wait_for_completion_interruptible(&buf->completion);
+ ret = fw_umh_wait_timeout(&buf->fw_umh, 0);
mutex_lock(&fw_lock);
}
and here we here we wait for it.
So I suggest to rename it back to fw_status_ and don't move it inside a
CONFIG_FW_LOADER_HELPER section. Drop the special handling of the
ABORTED and add instead a comment that the ABORTED state is not used for
direct loading. This special handling makes unnecessary more complex.
This is a slowpath and this micro optimization is helping to maintain
the code.
cheers,
daniel
