From: Takashi Iwai <firstname.lastname@example.org> To: Luis Chamberlain <email@example.com> Cc: Scott Branden <firstname.lastname@example.org>, Greg Kroah-Hartman <email@example.com>, Andy Gross <firstname.lastname@example.org>, David Brown <email@example.com>, Alexander Viro <firstname.lastname@example.org>, Shuah Khan <email@example.com>, firstname.lastname@example.org, "Rafael J . Wysocki" <email@example.com>, firstname.lastname@example.org, email@example.com, firstname.lastname@example.org, BCM Kernel Feedback <email@example.com>, Olof Johansson <firstname.lastname@example.org>, Andrew Morton <email@example.com>, Dan Carpenter <firstname.lastname@example.org>, Colin Ian King <email@example.com>, Kees Cook <firstname.lastname@example.org>, email@example.com Subject: Re: [PATCH 3/3] firmware: add mutex fw_lock_fallback for race condition Date: Fri, 23 Aug 2019 12:31:40 +0200 [thread overview] Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <20190820012655.GU16384@42.do-not-panic.com> 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. 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. (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. 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
next prev parent reply other threads:[~2019-08-23 10:31 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-16 0:09 [PATCH 0/3] firmware: selftest for request_firmware_into_buf Scott Branden 2019-08-16 0:09 ` [PATCH 1/3] test_firmware: add support " Scott Branden 2019-08-19 5:24 ` Luis Chamberlain 2019-08-19 20:27 ` shuah 2019-08-16 0:09 ` [PATCH 2/3] selftest: firmware: Add request_firmware_into_buf tests Scott Branden 2019-08-19 5:24 ` Luis Chamberlain 2019-08-19 20:27 ` shuah 2019-08-16 0:09 ` [PATCH 3/3] firmware: add mutex fw_lock_fallback for race condition Scott Branden 2019-08-19 5:39 ` Luis Chamberlain 2019-08-19 16:19 ` Scott Branden 2019-08-20 1:26 ` Luis Chamberlain 2019-08-20 15:54 ` Scott Branden 2019-08-23 10:31 ` Takashi Iwai [this message] 2019-08-23 15:43 ` Luis Chamberlain 2019-08-23 19:56 ` Scott Branden 2019-08-23 19:48 ` Scott Branden
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 3/3] firmware: add mutex fw_lock_fallback for race condition' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).