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

[leedom@chelsio.com (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
>