Re: wl1251 & mac address & calibration data
On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote: For the new API a solution for "fallback mechanisms" should be clean though and I am looking to stay as far as possible from the existing mess. A solution to help both the old API and new API is possible for the "fallback mechanism" though -- but for that I can only refer you at this point to some of Daniel Wagner and Tom Gunderson's firmwared deamon prospect. It should help pave the way for a clean solution and help address other stupid issues. The firmwared project is hosted here https://github.com/teg/firmwared As Luis pointed out, firmwared relies on FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default. I don't see any reason why firmwared should not also support loading calibration data. If we find a sound way to do this. As you can see from the commit history it is a pretty young project and more ore less reanimation of the old udev firmware loader feature. We are getting int into shape, adding integration tests etc. The main motivation for this project is the get movement back in stuck discussion on the firmware loader API. Luis was very busy writing up all the details on the current situation and purely from the amount of documentation need to describe the API you can tell something is awry. Thanks, Daniel
[PATCH 2/2] ath10k: use complete() instead complete_all()
From: Daniel Wagner <daniel.wag...@bmw-carit.de> There is only one waiter for the completion, therefore there is no need to use complete_all(). Let's make that clear by using complete() instead of complete_all(). The usage pattern of the completion is: waiter context waker context scan.started ath10k_start_scan() lockdep_assert_held(conf_mutex) auth10k_wmi_start_scan() wait_for_completion_timeout(scan.started) ath10k_wmi_event_scan_start_failed() complete(scan.started) ath10k_wmi_event_scan_started() complete(scan.started) scan.completed -- ath10k_scan_stop() lockdep_assert_held(conf_mutex) ath10k_wmi_stop_scan() wait_for_completion_timeout(scan.completed) __ath10k_scan_finish() complete(scan.completed) scan.on_channel --- ath10k_remain_on_channel() mutex_lock(conf_mutex) ath10k_start_scan() wait_for_completion_timeout(scan.on_channel) ath10k_wmi_event_scan_foreign_chan() complete(scan.on_channel) offchan_tx_completed ath10k_offchan_tx_work() mutex_lock(conf_mutex) reinit_completion(offchan_tx_completed) wait_for_completion_timeout(offchan_tx_completed) ath10k_report_offchain_tx() complete(offchan_tx_completed) install_key_done ath10k_install_key() lockep_assert_held(conf_mutex) reinit_completion(install_key_done) wait_for_completion_timeout(install_key_done) ath10k_htt_t2h_msg_handler() complete(install_key_done) vdev_setup_done --- ath10k_monitor_vdev_start() lockdep_assert_held(conf_mutex) reinit_completion(vdev_setup_done) ath10k_vdev_setup_sync() wait_for_completion_timeout(vdev_setup_done) ath10k_wmi_event_vdev_start_resp() complete(vdev_setup_done) ath10k_monitor_vdev_stop() lockdep_assert_held(conf_mutex) reinit_completion(vdev_setup_done() ath10k_vdev_setup_sync() wait_for_completion_timeout(vdev_setup_done) ath10k_wmi_event_vdev_stopped() complete(vdev_setup_done) thermal.wmi_sync ath10k_thermal_show_temp() mutex_lock(conf_mutex) reinit_completion(thermal.wmi_sync) wait_for_completion_timeout(thermal.wmi_sync) ath10k_thermal_event_temperature() complete(thermal.wmi_sync) bss_survey_done --- ath10k_mac_update_bss_chan_survey lockdep_assert_held(conf_mutex) reinit_completion(bss_survey_done) wait_for_completion_timeout(bss_survey_done) ath10k_wmi_event_pdev_bss_chan_info() complete(bss_survey_done) All complete() calls happen while the conf_mutex is taken. That means at max one waiter is possible. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/net/wireless/ath/ath10k/core.c | 16 drivers/net/wireless/ath/ath10k/mac.c | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index e889829..ed76601 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1497,14 +1497,14 @@ static void ath10k_core_restart(struct work_struct *work) ieee80211_stop_queues(ar->hw); ath10k_drain_tx(ar); - complete_all(>scan.started); - complete_all(>scan.completed); - complete_all(>scan.on_channel); - complete_all(>offchan_tx_completed); - complete_all(>install_key_done); - complete_all(>vdev_setup_done); - complete_all(>thermal.wmi_sync); - complete_all(>bss_survey_done); + complete(>scan.started); + complete(>scan.completed); + complete(>scan.on_channel); + complete(>offchan_tx_completed); + complete(>install_key_done); + complete(>vdev_setup_done); + complete(>thermal.wmi_sync); + complete(>bss_survey_done); wake_up(>htt.empty_tx_wq); wake_up(>wmi.tx_credits_wq); wake_up(>peer_mapping_wq); diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 0bbd0a0..c3c1c25 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k
[PATCH 0/2] wireless: Use complete() instead complete_all()
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Hi, Using complete_all() is not wrong per se but it suggest that there might be more than one reader. For -rt I am reviewing all complete_all() users and would like to leave only the real ones in the tree. The main problem for -rt about complete_all() is that it can be uses inside IRQ context and that can lead to unbounded amount work inside the interrupt handler. That is a no no for -rt. The patches grouped per subsystem and in small batches to allow reviewing. This series ignores all complete_all() usages in the firmware loading path. They will be hopefully address by Luis' sysdata patches [0]. That leaves a couple of complete_all() calls. The first patch fixes a real glitch for the carl9170 driver. I was able to test it because I have the hardware. For the second one I haven't found any dongle with that chip in my drawers. This series against net-next of today. cheers, daniel [0] https://lkml.kernel.org/r/1466117661-22075-1-git-send-email-mcg...@kernel.org Daniel Wagner (2): carl9170: Fix wrong completion usage ath10k: use complete() instead complete_all() drivers/net/wireless/ath/ath10k/core.c | 16 drivers/net/wireless/ath/ath10k/mac.c | 2 +- drivers/net/wireless/ath/carl9170/usb.c | 6 ++ 3 files changed, 11 insertions(+), 13 deletions(-) -- 2.7.4
[PATCH 1/2] carl9170: Fix wrong completion usage
From: Daniel Wagner <daniel.wag...@bmw-carit.de> carl9170_usb_stop() is used from several places to flush and cleanup any pending work. The normal pattern is to send a request and wait for the irq handler to call complete(). The completion is not reinitialized during normal operation and as the old comment indicates it is important to keep calls to wait_for_completion_timeout() and complete() balanced. Calling complete_all() brings this equilibirum out of balance and needs to be fixed by a reinit_completion(). But that opens a small race window. It is possible that the sequence of complete_all(), reinit_completion() is faster than the wait_for_completion_timeout() can do its work. The wake up is not lost but the done counter test is after reinit_completion() has been executed. The only reason we don't see carl9170_exec_cmd() hang forever is we use the timeout version of wait_for_copletion(). Let's fix this by reinitializing the completion (that is just setting done counter to 0) just before we send out an request. Now, carl9170_usb_stop() can be sure a complete() call is enough to make progess since there is only one waiter at max. This is a common pattern also seen in various drivers which use completion. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/net/wireless/ath/carl9170/usb.c | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c index 76842e6..99ab203 100644 --- a/drivers/net/wireless/ath/carl9170/usb.c +++ b/drivers/net/wireless/ath/carl9170/usb.c @@ -670,6 +670,7 @@ int carl9170_exec_cmd(struct ar9170 *ar, const enum carl9170_cmd_oids cmd, ar->readlen = outlen; spin_unlock_bh(>cmd_lock); + reinit_completion(>cmd_wait); err = __carl9170_exec_cmd(ar, >cmd, false); if (!(cmd & CARL9170_CMD_ASYNC_FLAG)) { @@ -778,10 +779,7 @@ void carl9170_usb_stop(struct ar9170 *ar) spin_lock_bh(>cmd_lock); ar->readlen = 0; spin_unlock_bh(>cmd_lock); - complete_all(>cmd_wait); - - /* This is required to prevent an early completion on _start */ - reinit_completion(>cmd_wait); + complete(>cmd_wait); /* * Note: -- 2.7.4
Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
On 08/02/2016 09:41 AM, Luis R. Rodriguez wrote: On Tue, Aug 02, 2016 at 08:53:55AM +0200, Daniel Wagner wrote: On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote: On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote: So you argue for the remoteproc use case with 100+ MB firmware that if there is a way to load after pivot_root() (or other additional firmware partition shows up) then there is no need at all for usermode helper? No, I'm saying I'd like to hear valid uses cases for the usermode helper and so far I have only found using coccinelle grammar 2 explicit users, that's it. My patch series (not yet merge) then annotates these as valid as I've verified through their documentation they have some quirky requirement. I got that question wrong. It should read something like 'for the remoteproc 100+MB there is no need for the user help?'. I've gone through your patches and they make perfectly sense too. Maybe I can convince you to take a better version of my patch 3 into your queue. And I help you converting the exiting drivers. Obviously if you like my help at all. Other than these two drivers I'd like hear to valid requirements for it. The existential issue is a real issue but it does not look impossible to resolve. It may be a solution to bloat up the kernel with 100+ MB size just to stuff built-in firmware to avoid this issue, but it does not mean a solution is not possible. Remind me -- why can remoteproc not stuff the firmware in initramfs ? I don't know. I was just bringing it up with the hope that Bjorn will defend it. It seems my tactics didn't work out :) Anyway, here's a simple suggestion: fs/exec.c gets a sentinel file monitor support per enum kernel_read_file_id. For instance we'd have one for READING_FIRMWARE, one for READING_KEXEC_IMAGE, perhaps READING_POLICY, and this would in turn be used as the system configurable deterministic file for which to wait for to be present before enabling each enum kernel_read_file_id type read. Thoughts ? Not sure if I get you here correctly. Is the 'system configurable deterministic file' is a knob which controlled by user space? Or it this something you define at compile time? Hmm, so it would allow to decided to ask a userspace helper or load the firmware directly (to be more precised the kernel_read_file_id type). If yes, than it is what currently already have just integrated nicely into the new sysdata API. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
On 08/02/2016 08:34 AM, Luis R. Rodriguez wrote: On Tue, Aug 02, 2016 at 07:49:19AM +0200, Daniel Wagner wrote: The sysdata API's main goal rather is to provide a flexible API first, compartamentalizing the usermode helper was secondary. But now it seems I may just also add devm support too to help simplify code further. I missed the point that you plan to add usermode helper support to the sysdata API. I had no such plans, when I have asked folks so far about "hey are you really in need for it, OK what for? " and "what extended uses do you envision?" so I far I have not gotten any replies at all. So -- instead sysdata currently ignores it. So you argue for the remoteproc use case with 100+ MB firmware that if there is a way to load after pivot_root() (or other additional firmware partition shows up) then there is no need at all for usermode helper? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
Hi Luis, I was ignorant on all the nasty details around the firmware loading. If I parse Luis' patches correctly they introduce an API which calls kernel_read_file_from_path() asynchronously: sysdata_file_request_async(..., ) *coookie = async_schedule_domain(request_sysdata_file_work_func(), ..) request_sysdata_file_work_fun() _sysdata_file_request() fw_get_filesystem_firmware() kernel_read_file_from_path() sysdata_synchronize_request(); Doesn't look like what your asking for. No, but its also a generic kernel read issue as I noted in my last reply. Okay, got it. If we want to overhaul firmware loading support we need to figure out how to support case when a driver want to [asynchronously] request firmware/config/blob and the rest of the system is not ready. Even if we want kernel to do read/load the data we need userspace to tell kernel when firmware partition is available, until then the kernel should not fail the request. I gather from Luis' blog post and comments that he is on the quest on removing userspace support completely. No, I explained in my last proposed documentation patch series that we cannot get rid of the usermode helper. I stand corrected. Its not well understood why so I explained and documented why. Obviously, I got lost somewhere there :) Best we can do is compartamentalize its uses. Sounds like a plan. The sysdata API's main goal rather is to provide a flexible API first, compartamentalizing the usermode helper was secondary. But now it seems I may just also add devm support too to help simplify code further. I missed the point that you plan to add usermode helper support to the sysdata API. What Dmitry notes is an existential issue with kernel_read_file_from_path() and we need a common solution for it. Understood. I guess best thing to keep that discussion in the other subthread. Maybe this attempt here could be a step before. Step 1 would be changing request_firmware_nowait() to request_firmware_async so drivers don't have to come up with their own sync primitives, e.g. cookie = request_firmware_async() fw_load_wait(cookie) That's one of the features already part of async mechanism of the sysdata API :) Yes, I realized that too :) cheers, daniel Thanks for the feedback. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
On 07/31/2016 09:23 AM, Dmitry Torokhov wrote: On July 30, 2016 9:58:17 AM PDT, "Luis R. Rodriguez" <mcg...@kernel.org> wrote: On Sat, Jul 30, 2016 at 02:42:41PM +0200, Arend van Spriel wrote: On 29-07-16 08:13, Daniel Wagner wrote: On 07/28/2016 09:01 PM, Bjorn Andersson wrote: On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote: + Luis (again) ;-) That was not on purpose :) My attempt to keep the Cc list a bit shorter was a failure. Do not quite like it... I'd rather asynchronous request give out firmware status pointer that could be used later on. Excellent. Why not get rid of the callback function as well and have fw_loading_wait() return result (0 = firmware available, < 0 = fail). Just to confirm, you are proposing a new API function next to request_firmware_nowait(), right? If proposing new firmware_class patches please bounce / Cc me, I've recently asked for me to be added to MAINTAINERS so I get these e-mails as I'm working on a new flexible API which would allow us to extend the firmware API without having to care about the old stupid usermode helper at all. These patches here are a first attempt to clean up a bit of the code around the completion API. As Dmitry correctly pointed out, it makes more sense to go bit further and make the async loading a bit more convenient for the drivers. I am not sure why we started calling usermode helper "stupid". We only had to implement direct kernel firmware loading because udev/stsremd folks had "interesting" ideas how events should be handled; but having userspace to feed us data is not stupid. I was ignorant on all the nasty details around the firmware loading. If I parse Luis' patches correctly they introduce an API which calls kernel_read_file_from_path() asynchronously: sysdata_file_request_async(..., ) *coookie = async_schedule_domain(request_sysdata_file_work_func(), ..) request_sysdata_file_work_fun() _sysdata_file_request() fw_get_filesystem_firmware() kernel_read_file_from_path() sysdata_synchronize_request(); Doesn't look like what your asking for. If we want to overhaul firmware loading support we need to figure out how to support case when a driver want to [asynchronously] request firmware/config/blob and the rest of the system is not ready. Even if we want kernel to do read/load the data we need userspace to tell kernel when firmware partition is available, until then the kernel should not fail the request. I gather from Luis' blog post and comments that he is on the quest on removing userspace support completely. Maybe this attempt here could be a step before. Step 1 would be changing request_firmware_nowait() to request_firmware_async so drivers don't have to come up with their own sync primitives, e.g. cookie = request_firmware_async() fw_load_wait(cookie) Step 2 could be something more towards sysdata approach. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
On 07/28/2016 09:01 PM, Bjorn Andersson wrote: On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote: On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote: From: Daniel Wagner <daniel.wag...@bmw-carit.de> [..] Do not quite like it... I'd rather asynchronous request give out a firmware status pointer that could be used later on. pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME, pcu, ims_pcu_process_async_firmware); if (IS_ERR(pcu->fw_st)) return PTR_ERR(pcu->fw_st); fw_loading_wait(pcu->fw_st); In the remoteproc case (patch 6) this would clean up the code, rather than replacing the completion API 1 to 1. I like it! IIRC most drivers do it the same way. So request_firmware_async() indeed would be good thing to have. Let me try that. Thanks for the excellent feedback. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
On 07/28/2016 07:57 PM, Dmitry Torokhov wrote: On Thu, Jul 28, 2016 at 09:55:07AM +0200, Daniel Wagner wrote: +int __firmware_stat_wait(struct firmware_stat *fwst, + long timeout) +{ + int err; + err = swait_event_interruptible_timeout(fwst->wq, + is_fw_sync_done(READ_ONCE(fwst->status)), + timeout); + if (err == 0 && fwst->status == FW_STATUS_ABORT) + return -ENOENT; + + return err; +} +EXPORT_SYMBOL(__firmware_stat_wait); + +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status) +{ + WRITE_ONCE(fwst->status, status); + swake_up(>wq); Do we need to notify everyone for FW_STATUS_LOADING status? Hmm, I don't think so. In the end drivers are probably only interested in the final result which is either success or fail. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 3/8] firmware: Factor out firmware load helpers
It's somewhat odd to me that the structure is "firmware_stat" but most of the functions are "fw_loading_*". That seems inconsistent for a structure that is (currently) only used by these functions. I agree, my proposal is odd. I would personally do either: a) "struct fw_load_status" and "fw_load_*()" or b) "struct firmware_load_stat" and "firmware_load_*()" I'd also change the function names from "loading" -> "load", similar to how userland has read(2), not reading(2). a) sounds good to me, because fw_load_ should be long enough prefix. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
Looking at the API, I really don't like the mixing of namespaces. Either it's fw_ or it's firmware_ but not a mix of both. I agree, that was not really clever. struct fw_loading { ... } __fw_loading_*() fw_loading_*() Would that make more sense? firmware_loading_ as prefix is a bit long IMO. Also looks like fw_loading_start() would do nothing as the struct is likely zero initialised, and FW_STATUS_LOADING == 0. Maybe you need an UNSET enum member? Good point, I cut a corner here a bit :). In the spirit of making things more clear fw_loading_start() should be used. FW_STATUS_ABORT <- FW_STATUS_ABORTED, to match the adjective suffixes of the other members. Ditto fw_loading_abort() which doesn't abort firmware loading but sets the status. Okay. Thanks a lot for the feedback. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
On 07/28/2016 01:22 PM, Bastien Nocera wrote: On Thu, 2016-07-28 at 09:55 +0200, Daniel Wagner wrote: From: Daniel Wagner <daniel.wag...@bmw-carit.de> Loading firmware is an operation many drivers implement in various ways around the completion API. And most of them do it almost in the same way. Let's reuse the firmware_stat API which is used also by the firmware_class loader. Apart of streamlining the firmware loading states we also document it slightly better. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> Irina added and tested that feature, so best for her to comment on this, as I don't have any hardware that would use this feature. In case you have any comments on the API, let me know. I'll add Irina to the Cc list in the next version. cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 2/8] selftests: firmware: do not clutter output
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Some of the test are supposed to fail but we get a few ouptput lines: ./fw_filesystem.sh: line 51: printf: write error: Invalid argument ./fw_filesystem.sh: line 56: printf: write error: No such device ./fw_filesystem.sh: line 62: echo: write error: No such file or directory Let's silence them so that the selftest output is clean if all is fine. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- tools/testing/selftests/firmware/fw_filesystem.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh index 5c495ad..d8ac9ba 100755 --- a/tools/testing/selftests/firmware/fw_filesystem.sh +++ b/tools/testing/selftests/firmware/fw_filesystem.sh @@ -48,18 +48,18 @@ echo "ABCD0123" >"$FW" NAME=$(basename "$FW") -if printf '\000' >"$DIR"/trigger_request; then +if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then echo "$0: empty filename should not succeed" >&2 exit 1 fi -if printf '\000' >"$DIR"/trigger_async_request; then +if printf '\000' >"$DIR"/trigger_async_request 2> /dev/null; then echo "$0: empty filename should not succeed (async)" >&2 exit 1 fi # Request a firmware that doesn't exist, it should fail. -if echo -n "nope-$NAME" >"$DIR"/trigger_request; then +if echo -n "nope-$NAME" >"$DIR"/trigger_request 2> /dev/null; then echo "$0: firmware shouldn't have loaded" >&2 exit 1 fi -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 5/8] ath9k_htc: use firmware_stat instead of completion
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Loading firmware is an operation many drivers implement in various ways around the completion API. And most of them do it almost in the same way. Let's reuse the firmware_stat API which is used also by the firmware_class loader. Apart of streamlining the firmware loading states we also document it slightly better. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/net/wireless/ath/ath9k/hif_usb.c | 10 +- drivers/net/wireless/ath/ath9k/hif_usb.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c index e1c338c..0a05d68 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.c +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c @@ -1067,7 +1067,7 @@ static void ath9k_hif_usb_firmware_fail(struct hif_device_usb *hif_dev) struct device *dev = _dev->udev->dev; struct device *parent = dev->parent; - complete_all(_dev->fw_done); + fw_loading_abort(hif_dev->fw_st); if (parent) device_lock(parent); @@ -1192,7 +1192,7 @@ static void ath9k_hif_usb_firmware_cb(const struct firmware *fw, void *context) release_firmware(fw); hif_dev->flags |= HIF_USB_READY; - complete_all(_dev->fw_done); + fw_loading_done(hif_dev->fw_st); return; @@ -1287,7 +1287,7 @@ static int ath9k_hif_usb_probe(struct usb_interface *interface, #endif usb_set_intfdata(interface, hif_dev); - init_completion(_dev->fw_done); + firmware_stat_init(_dev->fw_st); ret = ath9k_hif_request_firmware(hif_dev, true); if (ret) @@ -1330,7 +1330,7 @@ static void ath9k_hif_usb_disconnect(struct usb_interface *interface) if (!hif_dev) return; - wait_for_completion(_dev->fw_done); + fw_loading_wait(hif_dev->fw_st); if (hif_dev->flags & HIF_USB_READY) { ath9k_htc_hw_deinit(hif_dev->htc_handle, unplugged); @@ -1363,7 +1363,7 @@ static int ath9k_hif_usb_suspend(struct usb_interface *interface, if (!(hif_dev->flags & HIF_USB_START)) ath9k_htc_suspend(hif_dev->htc_handle); - wait_for_completion(_dev->fw_done); + fw_loading_wait(hif_dev->fw_st); if (hif_dev->flags & HIF_USB_READY) ath9k_hif_usb_dealloc_urbs(hif_dev); diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.h b/drivers/net/wireless/ath/ath9k/hif_usb.h index 7c2ef7e..0af9fe4 100644 --- a/drivers/net/wireless/ath/ath9k/hif_usb.h +++ b/drivers/net/wireless/ath/ath9k/hif_usb.h @@ -111,7 +111,7 @@ struct hif_device_usb { const struct usb_device_id *usb_device_id; const void *fw_data; size_t fw_size; - struct completion fw_done; + struct firmware_stat fw_st; struct htc_target *htc_handle; struct hif_usb_tx tx; struct usb_anchor regout_submitted; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 8/8] iwl4965: use firmware_stat instead of completion
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Loading firmware is an operation many drivers implement in various ways around the completion API. And most of them do it almost in the same way. Let's reuse the firmware_stat API which is used also by the firmware_class loader. Apart of streamlining the firmware loading states we also document it slightly better. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/net/wireless/intel/iwlegacy/4965-mac.c | 8 drivers/net/wireless/intel/iwlegacy/common.h | 3 ++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/net/wireless/intel/iwlegacy/4965-mac.c b/drivers/net/wireless/intel/iwlegacy/4965-mac.c index a91d170..d5e5808 100644 --- a/drivers/net/wireless/intel/iwlegacy/4965-mac.c +++ b/drivers/net/wireless/intel/iwlegacy/4965-mac.c @@ -5005,7 +5005,7 @@ il4965_ucode_callback(const struct firmware *ucode_raw, void *context) /* We have our copies now, allow OS release its copies */ release_firmware(ucode_raw); - complete(>_4965.firmware_loading_complete); + fw_loading_done(il->_4965.fw_st); return; try_again: @@ -5019,7 +5019,7 @@ err_pci_alloc: IL_ERR("failed to allocate pci memory\n"); il4965_dealloc_ucode_pci(il); out_unbind: - complete(>_4965.firmware_loading_complete); + fw_loading_done(il->_4965.fw_st); device_release_driver(>pci_dev->dev); release_firmware(ucode_raw); } @@ -6678,7 +6678,7 @@ il4965_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent) il_power_initialize(il); - init_completion(>_4965.firmware_loading_complete); + firmware_stat_init(>_4965.fw_st); err = il4965_request_firmware(il, true); if (err) @@ -6716,7 +6716,7 @@ il4965_pci_remove(struct pci_dev *pdev) if (!il) return; - wait_for_completion(>_4965.firmware_loading_complete); + fw_loading_wait(il->_4965.fw_st); D_INFO("*** UNLOAD DRIVER ***\n"); diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h index 726ede3..94af7b7 100644 --- a/drivers/net/wireless/intel/iwlegacy/common.h +++ b/drivers/net/wireless/intel/iwlegacy/common.h @@ -32,6 +32,7 @@ #include #include #include +#include #include #include @@ -1357,7 +1358,7 @@ struct il_priv { bool last_phy_res_valid; u32 ampdu_ref; - struct completion firmware_loading_complete; + struct firmware_stat fw_st; /* * chain noise reset and gain commands are the -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 6/8] remoteproc: use firmware_stat instead of completion
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Loading firmware is an operation many drivers implement in various ways around the completion API. And most of them do it almost in the same way. Let's reuse the firmware_stat API which is used also by the firmware_class loader. Apart of streamlining the firmware loading states we also document it slightly better. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/remoteproc/remoteproc_core.c | 10 +- drivers/soc/ti/wkup_m3_ipc.c | 2 +- include/linux/remoteproc.h | 6 -- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index db3958b..3b158f7 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -936,7 +936,7 @@ static void rproc_fw_config_virtio(const struct firmware *fw, void *context) out: release_firmware(fw); /* allow rproc_del() contexts, if any, to proceed */ - complete_all(>firmware_loading_complete); + fw_loading_done(rproc->fw_st); } static int rproc_add_virtio_devices(struct rproc *rproc) @@ -944,7 +944,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc) int ret; /* rproc_del() calls must wait until async loader completes */ - init_completion(>firmware_loading_complete); + firmware_stat_init(>fw_st); /* * We must retrieve early virtio configuration info from @@ -959,7 +959,7 @@ static int rproc_add_virtio_devices(struct rproc *rproc) rproc, rproc_fw_config_virtio); if (ret < 0) { dev_err(>dev, "request_firmware_nowait err: %d\n", ret); - complete_all(>firmware_loading_complete); + fw_loading_abort(rproc->fw_st); } return ret; @@ -1089,7 +1089,7 @@ static int __rproc_boot(struct rproc *rproc, bool wait) /* if rproc virtio is not yet configured, wait */ if (wait) - wait_for_completion(>firmware_loading_complete); + fw_loading_wait(rproc->fw_st); ret = rproc_fw_boot(rproc, firmware_p); @@ -1447,7 +1447,7 @@ int rproc_del(struct rproc *rproc) return -EINVAL; /* if rproc is just being registered, wait */ - wait_for_completion(>firmware_loading_complete); + fw_loading_wait(rproc->fw_st); /* clean up remote vdev entries */ list_for_each_entry_safe(rvdev, tmp, >rvdevs, node) diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c index 8823cc8..14c6396 100644 --- a/drivers/soc/ti/wkup_m3_ipc.c +++ b/drivers/soc/ti/wkup_m3_ipc.c @@ -370,7 +370,7 @@ static void wkup_m3_rproc_boot_thread(struct wkup_m3_ipc *m3_ipc) struct device *dev = m3_ipc->dev; int ret; - wait_for_completion(_ipc->rproc->firmware_loading_complete); + fw_loading_wait(m3_ipc->rproc->fw_st); init_completion(_ipc->sync_complete); diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h index 1c457a8..b8e7ff4 100644 --- a/include/linux/remoteproc.h +++ b/include/linux/remoteproc.h @@ -41,6 +41,7 @@ #include #include #include +#include /** * struct resource_table - firmware resource table header @@ -397,7 +398,7 @@ enum rproc_crash_type { * @num_traces: number of trace buffers * @carveouts: list of physically contiguous memory allocations * @mappings: list of iommu mappings we initiated, needed on shutdown - * @firmware_loading_complete: marks e/o asynchronous firmware loading + * @fw_sync: marks e/o asynchronous firmware loading * @bootaddr: address of first instruction to boot rproc with (optional) * @rvdevs: list of remote virtio devices * @notifyids: idr for dynamically assigning rproc-wide unique notify ids @@ -429,7 +430,7 @@ struct rproc { int num_traces; struct list_head carveouts; struct list_head mappings; - struct completion firmware_loading_complete; + struct firmware_stat fw_st; u32 bootaddr; struct list_head rvdevs; struct idr notifyids; @@ -479,6 +480,7 @@ struct rproc_vring { * @vring: the vrings for this vdev * @rsc_offset: offset of the vdev's resource entry */ + struct rproc_vdev { struct list_head node; struct rproc *rproc; -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 3/8] firmware: Factor out firmware load helpers
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Factor out the firmware loading synchronization code in order to allow drivers to reuse it. This also documents more clearly what is happening. This is especial useful the drivers which will be converted afterwards. Not everyone has to come with yet another way to handle it. We use swait instead completion. complete() would only wake up one waiter, so complete_all() is used. complete_all() wakes max MAX_UINT/2 waiters which is plenty in all cases. Though withh swait we just wait until the exptected status shows up and wake any waiter. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/base/firmware_class.c | 112 +++--- include/linux/firmware.h | 71 ++ 2 files changed, 122 insertions(+), 61 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index 773fc30..bf1ca70 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -30,6 +30,7 @@ #include #include #include +#include #include @@ -85,11 +86,36 @@ static inline bool fw_is_builtin_firmware(const struct firmware *fw) } #endif -enum { - FW_STATUS_LOADING, - FW_STATUS_DONE, - FW_STATUS_ABORT, -}; + +#if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && defined(MODULE)) + +static inline bool is_fw_sync_done(unsigned long status) +{ + return status == FW_STATUS_LOADED || status == FW_STATUS_ABORT; +} + +int __firmware_stat_wait(struct firmware_stat *fwst, + long timeout) +{ + int err; + err = swait_event_interruptible_timeout(fwst->wq, + is_fw_sync_done(READ_ONCE(fwst->status)), + timeout); + if (err == 0 && fwst->status == FW_STATUS_ABORT) + return -ENOENT; + + return err; +} +EXPORT_SYMBOL(__firmware_stat_wait); + +void __firmware_stat_set(struct firmware_stat *fwst, unsigned long status) +{ + WRITE_ONCE(fwst->status, status); + swake_up(>wq); +} +EXPORT_SYMBOL(__firmware_stat_set); + +#endif static int loading_timeout = 60; /* In seconds */ @@ -138,9 +164,8 @@ struct firmware_cache { struct firmware_buf { struct kref ref; struct list_head list; - struct completion completion; + struct firmware_stat fwst; struct firmware_cache *fwc; - unsigned long status; void *data; size_t size; #ifdef CONFIG_FW_LOADER_USER_HELPER @@ -194,7 +219,7 @@ static struct firmware_buf *__allocate_fw_buf(const char *fw_name, kref_init(>ref); buf->fwc = fwc; - init_completion(>completion); + firmware_stat_init(>fwst); #ifdef CONFIG_FW_LOADER_USER_HELPER INIT_LIST_HEAD(>pending_list); #endif @@ -292,15 +317,6 @@ static const char * const fw_path[] = { module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644); MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path"); -static void fw_finish_direct_load(struct device *device, - struct firmware_buf *buf) -{ - mutex_lock(_lock); - set_bit(FW_STATUS_DONE, >status); - complete_all(>completion); - mutex_unlock(_lock); -} - static int fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf) { @@ -339,7 +355,7 @@ static int fw_get_filesystem_firmware(struct device *device, } dev_dbg(device, "direct-loading %s\n", buf->fw_id); buf->size = size; - fw_finish_direct_load(device, buf); + fw_loading_done(buf->fwst); break; } __putname(path); @@ -457,12 +473,11 @@ static void __fw_load_abort(struct firmware_buf *buf) * There is a small window in which user can write to 'loading' * between loading done and disappearance of 'loading' */ - if (test_bit(FW_STATUS_DONE, >status)) + if (is_fw_loading_done(buf->fwst)) return; list_del_init(>pending_list); - set_bit(FW_STATUS_ABORT, >status); - complete_all(>completion); + fw_loading_abort(buf->fwst); } static void fw_load_abort(struct firmware_priv *fw_priv) @@ -475,9 +490,6 @@ static void fw_load_abort(struct firmware_priv *fw_priv) fw_priv->buf = NULL; } -#define is_fw_load_aborted(buf)\ - test_bit(FW_STATUS_ABORT, &(buf)->status) - static LIST_HEAD(pending_fw_head); /* reboot notifier for avoid deadlock with usermode_lock */ @@ -577,7 +589,7 @@ static ssize_t firmware_loading_show(struct device *dev, mutex_lock(_lock); if (fw_priv->buf) -
[RFC v0 7/8] Input: ims-pcu: use firmware_stat instead of completion
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Loading firmware is an operation many drivers implement in various ways around the completion API. And most of them do it almost in the same way. Let's reuse the firmware_stat API which is used also by the firmware_class loader. Apart of streamlining the firmware loading states we also document it slightly better. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/input/misc/ims-pcu.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c index 9c0ea36..cda1fbf 100644 --- a/drivers/input/misc/ims-pcu.c +++ b/drivers/input/misc/ims-pcu.c @@ -109,7 +109,7 @@ struct ims_pcu { u32 fw_start_addr; u32 fw_end_addr; - struct completion async_firmware_done; + struct firmware_stat fw_st; struct ims_pcu_buttons buttons; struct ims_pcu_gamepad *gamepad; @@ -940,7 +940,7 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw, release_firmware(fw); out: - complete(>async_firmware_done); + fw_loading_done(pcu->fw_st); } /* @@ -1967,7 +1967,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu) ims_pcu_process_async_firmware); if (error) { /* This error is not fatal, let userspace have another chance */ - complete(>async_firmware_done); + fw_loading_abort(pcu->fw_st); } return 0; @@ -1976,7 +1976,7 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu) static void ims_pcu_destroy_bootloader_mode(struct ims_pcu *pcu) { /* Make sure our initial firmware request has completed */ - wait_for_completion(>async_firmware_done); + fw_loading_wait(pcu->fw_st); } #define IMS_PCU_APPLICATION_MODE 0 @@ -2000,7 +2000,7 @@ static int ims_pcu_probe(struct usb_interface *intf, pcu->bootloader_mode = id->driver_info == IMS_PCU_BOOTLOADER_MODE; mutex_init(>cmd_mutex); init_completion(>cmd_done); - init_completion(>async_firmware_done); + firmware_stat_init(>fw_st); error = ims_pcu_parse_cdc_data(intf, pcu); if (error) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 0/8] Reuse firmware loader helpers
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Hi, While reviewing all the complete_all() users, I realized there is recouring pattern how the completion API is used to synchronize the stages of the firmware loading. Since firmware_class.c contains a fairly complete implemetation for synching the loading, it worthwhile to export it and reuse it in drivers, At the same time one complete_all() user is gone which is a good thing for -rt (*). The first 2 patches are bug fixes for the test script. Patch 3 adds the new API, and the following patches update a few drivers. I haven't updated all the drivers because I wanted to see first if this is going into the right direction. Since naming is a difficult, I am more than please to pick a better name if you have one. cheers, daniel (*) Under -rt waking all waiters via complete_all() is not good. If the complete_all() call happens in IRQ context we have an unbound latency there. Therefore the aim is to reduce the complete_all() users and get rid of them where possible. Daniel Wagner (8): selftests: firmware: do not abort test too early selftests: firmware: do not clutter output firmware: Factor out firmware load helpers Input: goodix: use firmware_stat instead of completion ath9k_htc: use firmware_stat instead of completion remoteproc: use firmware_stat instead of completion Input: ims-pcu: use firmware_stat instead of completion iwl4965: use firmware_stat instead of completion drivers/base/firmware_class.c | 112 ++ drivers/input/misc/ims-pcu.c | 10 +- drivers/input/touchscreen/goodix.c| 10 +- drivers/net/wireless/ath/ath9k/hif_usb.c | 10 +- drivers/net/wireless/ath/ath9k/hif_usb.h | 2 +- drivers/net/wireless/intel/iwlegacy/4965-mac.c| 8 +- drivers/net/wireless/intel/iwlegacy/common.h | 3 +- drivers/remoteproc/remoteproc_core.c | 10 +- drivers/soc/ti/wkup_m3_ipc.c | 2 +- include/linux/firmware.h | 71 ++ include/linux/remoteproc.h| 6 +- tools/testing/selftests/firmware/fw_filesystem.sh | 6 +- tools/testing/selftests/firmware/fw_userhelper.sh | 2 +- 13 files changed, 158 insertions(+), 94 deletions(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 1/8] selftests: firmware: do not abort test too early
From: Daniel Wagner <daniel.wag...@bmw-carit.de> When running the test script you will get: kselftest/firmware/fw_userhelper.sh: line 69: echo: write error: Resource temporarily unavailable and stops right there. Because the script runs with the '-e' option which will stop the script at any error. We should stop there because we are trying to something wrong and get an error reported back. Instead, ignore the error message and make sure we do not stop the script by setting the last error code to true. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- tools/testing/selftests/firmware/fw_userhelper.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/firmware/fw_userhelper.sh b/tools/testing/selftests/firmware/fw_userhelper.sh index b9983f8..2d244a7 100755 --- a/tools/testing/selftests/firmware/fw_userhelper.sh +++ b/tools/testing/selftests/firmware/fw_userhelper.sh @@ -66,7 +66,7 @@ NAME=$(basename "$FW") # Test failure when doing nothing (timeout works). echo 1 >/sys/class/firmware/timeout -echo -n "$NAME" >"$DIR"/trigger_request +echo -n "$NAME" >"$DIR"/trigger_request 2> /dev/null || true if diff -q "$FW" /dev/test_firmware >/dev/null ; then echo "$0: firmware was not expected to match" >&2 exit 1 -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v0 4/8] Input: goodix: use firmware_stat instead of completion
From: Daniel Wagner <daniel.wag...@bmw-carit.de> Loading firmware is an operation many drivers implement in various ways around the completion API. And most of them do it almost in the same way. Let's reuse the firmware_stat API which is used also by the firmware_class loader. Apart of streamlining the firmware loading states we also document it slightly better. Signed-off-by: Daniel Wagner <daniel.wag...@bmw-carit.de> --- drivers/input/touchscreen/goodix.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c index 240b16f..67158d3 100644 --- a/drivers/input/touchscreen/goodix.c +++ b/drivers/input/touchscreen/goodix.c @@ -47,7 +47,7 @@ struct goodix_ts_data { u16 id; u16 version; const char *cfg_name; - struct completion firmware_loading_complete; + struct firmware_stat fwst; unsigned long irq_flags; }; @@ -683,7 +683,7 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx) err_release_cfg: release_firmware(cfg); - complete_all(>firmware_loading_complete); + fw_loading_done(ts->fwst); } static int goodix_ts_probe(struct i2c_client *client, @@ -705,7 +705,7 @@ static int goodix_ts_probe(struct i2c_client *client, ts->client = client; i2c_set_clientdata(client, ts); - init_completion(>firmware_loading_complete); + firmware_stat_init(>fwst); error = goodix_get_gpio_config(ts); if (error) @@ -766,7 +766,7 @@ static int goodix_ts_remove(struct i2c_client *client) struct goodix_ts_data *ts = i2c_get_clientdata(client); if (ts->gpiod_int && ts->gpiod_rst) - wait_for_completion(>firmware_loading_complete); + fw_loading_wait(ts->fwst); return 0; } @@ -781,7 +781,7 @@ static int __maybe_unused goodix_suspend(struct device *dev) if (!ts->gpiod_int || !ts->gpiod_rst) return 0; - wait_for_completion(>firmware_loading_complete); + fw_loading_wait(ts->fwst); /* Free IRQ as IRQ pin is used as output in the suspend sequence */ goodix_free_irq(ts); -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html