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