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

[Daniel Wagner <daniel.wagner@bmw-carit.de>]

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@bmw-carit.de>
>>  		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