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

[Daniel Wagner]

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

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


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

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

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

cheers,
daniel