On 2019-08-23 3:31 a.m., Takashi Iwai wrote:
On Tue, 20 Aug 2019 03:26:55 +0200,
Luis Chamberlain wrote:
On Mon, Aug 19, 2019 at 09:19:51AM -0700, Scott Branden wrote:
To be honest, I find the entire firmware code sloppy.
And that is after years of cleanup on my part. Try going back to v4.1
for instance, check the code out then for an incredible horrific sight :)
I don't think the cache/no-cache feature is
implemented or tested properly nor fallback to begin with.
I'm in total agreement! I *know* there must be holes in that code, and I
acknowledge a few possible gotchas on the commit logs. For instance, I
acknowledged that the firmware cache had a secondary purpose which was
not well documented or understood through commit e44565f62a720
("firmware: fix batched requests - wake all waiters"). The firmware
cache allows for batching requests and sharing the same original request
for multiple consecutive requests which *race against each other*.
That's when I started having my doubts about the architecture of the
firmware cache mechanism, it seemed too complex and perhaps overkill
and considered killing it.
As I noted in that commit, the firmware cache is used for:
1) Addressing races with file lookups during the suspend/resume cycle by
keeping firmware in memory during the suspend/resume cycle
Right, this one is the significant need. And currently the fw loader
core takes a complicated approach as:
- Store firmware name string in devres for each firmware
- Upon suspend, loop over all devices and associated firmware names,
create a list, then loop over the list for loading the firmware
files before sleeping.
- Upon resume, release the firmware files that have been loaded at
suspend in a delayed manner.
So we have different level of lists there, which make the code quite
hard to understand.
The reason of the above approach is because we didn't know which
device driver would need the firmware at resume, so basically we do
cache for all devices. Maybe it'd better to look for the exact
drivers that require the firmware at resume, and handle only such
ones instead of catch-all approach.
Yes, that would be better. Or remove this cache mechanism entirely
and provide some helper functions of some sort to the limited
drivers that actually require such mechanism.
OTOH, I find it's not bad to keep the loaded firmware file names per
device and expose e.g. via sysfs. Currently we have no way to look at
which firmware files have been loaded afterwards; the only way to see
it is enabling some debug option and read through kernel messages.
(FWIW, I stumbled on this problem since I wanted to provide the split
kernel-firmware package on SUSE distro, and let the installer decide
which package to pick up.)
2) Batched requests for the same file rely only on work from the first
file lookup, which keeps the firmware in memory until the last
release_firmware() is called
IMO, this feature can be omitted if it makes things too complicated.
I guess it were added because we handle the fw caching in anyway.
There isn't a big need for this due to performance. If the
performance matters, such driver should re-use its own firmware by
itself.
Any simplifications would be appreciated.
I sure don't understand what the code is trying to do.
(snip)
3) I have a driver that uses request_firmware_into_buf and have multiple
instances of the driver
Cool, is the driver upstream?
loading the same firmware in parallel. Some of the data is not read
correctly in each instance.
Makes perfect sense considering the lack of testing I noted.
I haven't yet to reproduce this issue with the firmware test
That's because of batched firmware request mechanism.
but currently
have a mutex around the entire
call to request_firmware_into_buf in our driver.
I will take a look at this now.
Perhaps it is better at this point to add a mutex in
request_firmware_into_buf to make is entirely safe?
No, that is not sufficient, although it would also solve the
issue.
The mutex for request_firmware_into_buf() doesn't sound like a good
approach. Basically the direct fw loading should work in parallel
for the same firmware file. We might have some bug wrt cache stuff,
but it can be fixed properly.
However, the fw loading in fallback mode can't run in parallel for
the same file, per design -- no matter whether cached or not.
So, if any, we'd need put a mutex around the fallback loader code.
And, the mutex should be rather per device, not a global one.
Sure, whatever solves the issue. All I wish to do is read
part of file into a buffer specified.
Or we may trick it by appending the second parallel caller into the
same wait queue, but the code will be more complex, so I don't think
worth for it.
thanks,
Takashi
