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

[Daniel Wagner]

On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
> On Fri, Oct 07, 2016 at 01:41:21PM +0200, Daniel Wagner wrote:
> > 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.

I used 'no pending request' for what you describe below with first call. 
So yes, either the buffer is allocated for the given name or it's 
missing. It doesn't tell anything about the load status of the firmware.

> >   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()

I confused that one from _request_firmware_load().

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

So I think I found the source of the confusion about 
fw_umh_wait_timeout(). When providing a timeout value of 0 you get the 
wait_for_completion_interruptible() version.

> > > > > +#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.

See above.

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

Yes, even 'struct firmware_priv' is only available when 
CONFIG_FW_LOADER_USER_HELPER. Though fw_finish_direct_load() is used in 
the !UHM section. I think I still miss your point here.


> > > > > /* 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?

I don't think so. The only thing is the value of the fw_uhm_wait_timeout().

cheers,
daniel