Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1502992 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-10-18 15:40 +0200
    Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-10-19 00:00 +0200
      Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-10-19 11:10 +0200

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

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

[toc] | [next] | [standalone]

#1503386

On Tue, Oct 18, 2016 at 03:30:45PM +0200, Daniel Wagner wrote:
> On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
> 
> > >   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().

Right and wait_for_completion_interruptible() has no 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.
> 
> 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.

I fail to see that, how so? Note that 0 does is not allowed anyway:

static inline long firmware_loading_timeout(void)
{
        return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
}

  Luis

[toc] | [prev] | [next] | [standalone]

#1503595

On 10/18/2016 11:54 PM, Luis R. Rodriguez wrote:
> On Tue, Oct 18, 2016 at 03:30:45PM +0200, Daniel Wagner wrote:
>> On 10/10/2016 10:37 PM, Luis R. Rodriguez wrote:
>>
>>>>   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().
> 
> Right and wait_for_completion_interruptible() has no timeout.

All wait_for_completion_*() function are small wrappers around
a common implemention. I thought that would be a clever idea to
reuse here, but from our discussion I see it isn't. My bad.

static int fw_umh_wait_timeout(struct fw_umh *fw_umh, long timeout)
{
	int ret;

	ret = wait_for_completion_interruptible_timeout(&fw_umh->completion,
							timeout);
	if (ret != 0 && test_bit(FW_UMH_ABORTED, &fw_umh->status))
		return -ENOENT;

	return ret;
}


long __sched
wait_for_completion_interruptible_timeout(struct completion *x,
					  unsigned long timeout)
{
	return wait_for_common(x, timeout, TASK_INTERRUPTIBLE);
}

int __sched wait_for_completion_interruptible(struct completion *x)
{
	long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE);
	if (t == -ERESTARTSYS)
		return t;
	return 0;
}


I think it is far better to do something like:

static __fw_umh_wait_common(struct fw_umh *fw_umh, long timeout) { ... }

#define fw_umh_wait(fw_umh) __fw_umh_wait_common(fw_umh, MAX_SCHEDULE_TIMEOUT)
#define fw_umh_wait_timeout(fw_umh, timeout) __fw_umh_wait_common(fw_umh, timeout)

(The function prefixes will be different, since umh isn't right as discussed.)

>>> 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.
> 
> I fail to see that, how so? Note that 0 does is not allowed anyway:
> 
> static inline long firmware_loading_timeout(void)
> {
>         return loading_timeout > 0 ? loading_timeout * HZ : MAX_JIFFY_OFFSET;
> }

Correct. The fw_umh_wait_timeout(0) is hard coded in sync_cached_firmware_buf().
fw_umh_wait_timeout(fw_umh, firmware_loading_timeout()) is used
_request_firmware_load().

I'll update the series and hopefully we get this all sorted out in the new
version. 

cheers,
daniel

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web