From: "Luis R. Rodriguez" <mcgrof@kernel.org>
To: "Luis R. Rodriguez" <mcgrof@kernel.org>,
John Ewalt <jewalt@lgsinnovations.com>
Cc: yi1.li@linux.intel.com, atull@kernel.org,
gregkh@linuxfoundation.org, wagi@monom.org, dwmw2@infradead.org,
rafal@milecki.pl, arend.vanspriel@broadcom.com,
rjw@rjwysocki.net, moritz.fischer@ettus.com, pmladek@suse.com,
johannes.berg@intel.com, emmanuel.grumbach@intel.com,
luciano.coelho@intel.com, kvalo@codeaurora.org, luto@kernel.org,
takahiro.akashi@linaro.org, dhowells@redhat.com,
pjones@redhat.com, linux-kernel@vger.kernel.org,
linux-fpga@vger.kernel.org
Subject: Re: [PATCHv2 0/3] Enable no_cache flag to driver_data
Date: Wed, 24 May 2017 22:32:46 +0200 [thread overview]
Message-ID: <20170524203246.GD8951@wotan.suse.de> (raw)
In-Reply-To: <20170524190357.GB8951@wotan.suse.de>
On Wed, May 24, 2017 at 09:03:57PM +0200, Luis R. Rodriguez wrote:
> __fw_lookup_buf() really should be
> renamed to something that reflects this is a cache lookup.
Actually I take this back, other than the cache, note that when we
fw_lookup_and_allocate_buf() we first __fw_lookup_buf() but if the buf is not
there we allocate a new one and list_add() it to the cache regardless of
whether or not the cache thing has been enabled.
What this does then *also* other than caching for suspend/resume (which we
should document more formally) is to gather up contending lookups together
with one buf, and share the final status of just one lookup. If a buf is
found we fw_state_wait() on _request_firmware_prepare() until the buf clears.
John Ewalt recently reported some issues with loadng multiple files at the
same time. He also provided a patch. The swake_up() fix seems sensible
and would seem to have been caused by the swait transformation, but in
inspecting the other proposed changes it would seem we have had tons of
other lingering bugs which have probably existed for ages.
For instance, if there are pending requests for a leader request to send back
info, and one is about to complete but in the last moment on
assign_firmware_buf() fails, all the error paths lack a wake up call. As such
all pending requests may just wait and linger, and since none of these have
a timeout I would expect these to just linger forever. I'm not even sure we
kref_get() properly on the buf for pending requests when we are waiting for
a serialized request, ie, we might be able to take a buf underneath the nose
of a waiter.
Although some are new bugs, some seem to be really old bugs.
These are the sorts of issues I wish for a test driver to be able to uncover,
test and ensure we never regress again. This is also why I am being careful
about enabling a feature, we should *really* think things through well before
enabling on the new API.
[0] https://bugzilla.kernel.org/show_bug.cgi?id=195477
Luis
next prev parent reply other threads:[~2017-05-24 20:32 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-05-20 6:46 [PATCHv2 0/3] Enable no_cache flag to driver_data yi1.li
2017-05-20 6:46 ` [PATCHv2 1/3] firmware_class: move NO_CACHE from private to driver_data_req_params yi1.li
2017-05-20 6:46 ` [PATCHv2 2/3] iwlwifi: use DRIVER_DATA_REQ_NO_CACHE for driver_data yi1.li
2017-05-20 6:46 ` [PATCHv2 3/3] test: add no_cache to driver_data load tester yi1.li
2017-05-24 19:03 ` [PATCHv2 0/3] Enable no_cache flag to driver_data Luis R. Rodriguez
2017-05-24 20:32 ` Luis R. Rodriguez [this message]
2017-05-25 22:30 ` Li, Yi
2017-05-25 22:43 ` Luis R. Rodriguez
2017-05-26 21:05 ` Li, Yi
2017-05-26 21:13 ` Luis R. Rodriguez
2017-06-06 19:31 ` Li, Yi
2017-06-07 17:59 ` Luis R. Rodriguez
2017-06-07 21:00 ` Li, Yi
2017-06-07 23:02 ` Luis R. Rodriguez
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 \
--in-reply-to=20170524203246.GD8951@wotan.suse.de \
--to=mcgrof@kernel.org \
--cc=arend.vanspriel@broadcom.com \
--cc=atull@kernel.org \
--cc=dhowells@redhat.com \
--cc=dwmw2@infradead.org \
--cc=emmanuel.grumbach@intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jewalt@lgsinnovations.com \
--cc=johannes.berg@intel.com \
--cc=kvalo@codeaurora.org \
--cc=linux-fpga@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luciano.coelho@intel.com \
--cc=luto@kernel.org \
--cc=moritz.fischer@ettus.com \
--cc=pjones@redhat.com \
--cc=pmladek@suse.com \
--cc=rafal@milecki.pl \
--cc=rjw@rjwysocki.net \
--cc=takahiro.akashi@linaro.org \
--cc=wagi@monom.org \
--cc=yi1.li@linux.intel.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox