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

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Fri, Oct 07, 2016 at 01:41:21PM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> 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.
> > 
> > Where?
> 
> 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)
> 
> _request_firmware()
>   _request_firmware_prepare()
>     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.

> 
>   fw_get_fileystem_firmware()
>     fw_finish_direct_load()
>       complete_all()
> 
> 
> 2nd request (waiter context)
> 
> _request_firmware()
>   _request_firmware_prepare()
>      fw_lookup_allocate_buf()      # finds previously allocated buf
>                                    # returns 1 -> wait for loading
>      sync_cached_firmware_buf()
>         wait_for_completion_interruptible_timeout()

No, that's wait_for_completion_interruptible() not
           wait_for_completion_interruptible_timeout()

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.

> > > > +#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.
> > 
> > Why?
> 
> See above.

Sure, but note how the timeout is not used.

> > > > @@ -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
> > 
> > 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.

> > > > /* 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.
> > 
> > Likewise.
> 
> As I tried to explain above the buffering code is depending on completion.

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

  Luis