Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Casey Leedom]

  Okay, I'll leave the whole request_firmware{,_direct,_nowait}() thing 
alone.  The request_firmware_direct() will "solve" a non-problem (since 
all of our "firmware" files are _supposed to be_ always present.  (And 
the 60 second timeout for udev to confirm that a file doesn't exist 
seems like udev is just basically broken.)

  That aside, we still need to solve the real problem that we're 
experiencing in that the boot-time load of cxgb4 is timing out on SLE12 
because a maximum load timeout has been instituted in that distribution 
for driver modules and if there are two 10Gb/s-BT adapters present in a 
system which need both base firmware and BT PHY firmware, we exceed that 
timeout.  The timeout really should be per device (since there ~could~ 
be an arbitrary number of devices in a system) and there probably should 
be a way for the driver to notify the kernel timeout mechanism that 
forward progress is being made ...

Casey

On 06/25/14 10:31, Luis R. Rodriguez wrote:
> On Wed, Jun 25, 2014 at 10:12:20AM -0700, Casey Leedom wrote:
> > On 06/24/14 18:50, Luis R. Rodriguez wrote:
> > > On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote:
> > > > [[ Hopefully this makes it through to the kernel.org lists -- Iâm using the
> > > >     Mac OS/X Mailer and itâs not clear how to force it not to use HTML format.
> > > >     -- Casey ]]
> > > >
> > > >     So does request_firmware_direct() only fail if the requested file is not
> > > >     present on the file system or does it fail in other cases as well?
> > > Same as before they are the same exact call with the only difference
> > > being udev is not used as an extra helper, so it saves the extra
> > > delay caused by udev. That's all.
> > >
> > > >     If itâs the former, then the change to cxgb4 is fine.
> > > >
> > > >     But if itâs the latter, then itâs definitely not okay.  While the driver
> > > >     _can_ continue running without the local on-disk Firmware Configuration
> > > >     File, that file can be used to significantly change the behavior and
> > > >     capabilities of the adapter and is user-customizable.  If a user makes
> > > >     changes to the local on-disk Firmware Configuration File and these are
> > > >     randomly silently ignored this will lead to highly annoying support issues.
> > > This just avoids udev, the request goes directly to the filesystem. The
> > > failure will happen when the file is not present just as before, the
> > > only difference here is skipping udev, it doesn't suffer from the extra
> > > 60 second timeout. There's another possible failure, when
> > > usermodehelper_read_trylock() fails but that is just as the code was before
> > > so this change doesn't introduce that as a new false check. When that
> > > triggers yout get a nasty WARN_ON() just as before.
> >    Huh, okay.  I guess I'm confused about the value of request_firmware()
> > and the User Device helper.  If request_firmware_direct() just goes to the
> > file system to grab the file and returns with ENOENT if it's not there,
> > then you could replace every usage of request_firmware() in the cxgb4
> > driver as far as I can see ...  Either the files are there and we'll use
> > them or they won't be and we'll have to cope with that.  Am I missing
> > something?
> You're actually right specially given that udev firmware uploading will
> hopefully eventually be removed eventually (it seems it was just one driver
> that caused to consider waiting on the removal, some driver that required
> looking for firmware on some custom path I think or used a custom loader), for
> now however its best to keep things consistent otherwise we'd replace
> everything already. The _direct() call then is best used for optional firmware
> for now. Perhaps in the end will be that the non _direct() call will have an
> explicit print to the ring buffer if the file was not found.
>
> >    And again, this definitely isn't going to solve the problem that started
> > this whole line of research:
> I consider this research part of understanding and optimizing firmware
> loading on cxgb4, in this case this would save 60 seconds for each
> optional configuration file not present when loading, its not clear to
> me how often devices don't have optional configs so its unclear to me the
> exact savings in general, but if there's at least one user that should
> speed things up.
>
> > we're still going to time out the load of
> > cxgb4 if there are multiple 10Gb/s BT adapters in a system and we need to
> > load each one with both base firmware and PHY firmware.
> Again, the timeout is *within* firmware_request(), firmware_release() does
> not tell the firmware loader the timeout is over. The timeout is for the
> kernel doing the hunt for the file.
>
> As I see it the next steps on the evaluation on firmware loading on cxgb4
> would be to evaluate a clean strategy to split things up, and also would
> appreciate feedback on the bus master thing. Perhaps best we continue that
> discussoin on that thread?
>
>    Luis

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/