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:
- 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.)
- 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)
- 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