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
