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

[Daniel Wagner]

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