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

Groups > linux.kernel > #1496034 > 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 "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-10-05 22:30 +0200
    Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status Daniel Wagner <daniel.wagner@bmw-carit.de> - 2016-10-07 13:50 +0200
      Re: [PATCH v5 2/5] firmware: encapsulate firmware loading status "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-10-10 22:40 +0200

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

On Tue, Sep 13, 2016 at 11:47:08AM +0200, Daniel Wagner wrote:
> Hi Luis,
> 
> 
> 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?

> >+#ifdef CONFIG_FW_LOADER_USER_HELPER
> >+
> >+enum {
> >+	FW_UMH_UNKNOWN,
> >+	FW_UMH_LOADING,
> >+	FW_UMH_DONE,
> >+	FW_UMH_ABORTED,
> >+};
> 
> The direct loading path just uses two states, LOADING and DONE.
> ABORTED is not used.
> 
> >+struct fw_umh {
> >+	struct completion completion;
> >+	unsigned long status;
> >+};
> >+
> >+static void fw_umh_init(struct fw_umh *fw_umh)
> >+{
> >+	init_completion(&fw_umh->completion);
> >+	fw_umh->status = FW_UMH_UNKNOWN;
> >+}
> >+
> >+static int __fw_umh_check(struct fw_umh *fw_umh, unsigned long status)
> >+{
> >+	return test_bit(status, &fw_umh->status);
> >+}
> >+
> >+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;
> >+}
> >+
> >+static void __fw_umh_set(struct fw_umh *fw_umh,
> >+			  unsigned long status)
> >+{
> >+	set_bit(status, &fw_umh->status);
> >+
> >+	if (status == FW_UMH_DONE || status == FW_UMH_ABORTED) {
> >+		clear_bit(FW_UMH_LOADING, &fw_umh->status);
> >+		complete_all(&fw_umh->completion);
> >+	}
> >+}
> >+
> >+#define fw_umh_start(fw_umh)					\
> >+	__fw_umh_set(fw_umh, FW_UMH_LOADING)
> >+#define fw_umh_done(fw_umh)					\
> >+	__fw_umh_set(fw_umh, FW_UMH_DONE)
> >+#define fw_umh_aborted(fw_umh)					\
> >+	__fw_umh_set(fw_umh, FW_UMH_ABORTED)
> >+#define fw_umh_is_loading(fw_umh)				\
> >+	__fw_umh_check(fw_umh, FW_UMH_LOADING)
> >+#define fw_umh_is_done(fw_umh)					\
> >+	__fw_umh_check(fw_umh, FW_UMH_DONE)
> >+#define fw_umh_is_aborted(fw_umh)				\
> >+	__fw_umh_check(fw_umh, FW_UMH_ABORTED)
> >+
> >+#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?

> fw_umh_is_aborted() is not
> needed.
> 
> 
> >@@ -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?

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

  Luis

[toc] | [next] | [standalone]

#1497131

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

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

#1498524

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web