[PATCH 0/2] ath10k: async pci probing

[PATCH 0/2] ath10k: async pci probing

[Michal Kazior]

Hi,

Recently I've tried to built ath10k into kernel.
Thus I've discovered it doesn't play nice.

Are there any gotchas I should be aware of and
might've missed while changing this sort of thing?


Michal Kazior (2):
  ath10k: relocate core create/destroy functions
  ath10k: make core registering async

 drivers/net/wireless/ath/ath10k/core.c | 170 ++++++++++++++++++---------------
 drivers/net/wireless/ath/ath10k/core.h |   2 +
 drivers/net/wireless/ath/ath10k/pci.c  |   2 -
 3 files changed, 96 insertions(+), 78 deletions(-)

-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/2] ath10k: relocate core create/destroy functions

[Michal Kazior]

This will avoid unnecessary forward declaration of
any kind in the future.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 128 ++++++++++++++++-----------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 75b3dfb..bd4d1cc 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -701,70 +701,6 @@ static void ath10k_core_restart(struct work_struct *work)
 	mutex_unlock(&ar->conf_mutex);
 }
 
-struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
-				  const struct ath10k_hif_ops *hif_ops)
-{
-	struct ath10k *ar;
-
-	ar = ath10k_mac_create();
-	if (!ar)
-		return NULL;
-
-	ar->ath_common.priv = ar;
-	ar->ath_common.hw = ar->hw;
-
-	ar->p2p = !!ath10k_p2p;
-	ar->dev = dev;
-
-	ar->hif.priv = hif_priv;
-	ar->hif.ops = hif_ops;
-
-	init_completion(&ar->scan.started);
-	init_completion(&ar->scan.completed);
-	init_completion(&ar->scan.on_channel);
-	init_completion(&ar->target_suspend);
-
-	init_completion(&ar->install_key_done);
-	init_completion(&ar->vdev_setup_done);
-
-	setup_timer(&ar->scan.timeout, ath10k_reset_scan, (unsigned long)ar);
-
-	ar->workqueue = create_singlethread_workqueue("ath10k_wq");
-	if (!ar->workqueue)
-		goto err_wq;
-
-	mutex_init(&ar->conf_mutex);
-	spin_lock_init(&ar->data_lock);
-
-	INIT_LIST_HEAD(&ar->peers);
-	init_waitqueue_head(&ar->peer_mapping_wq);
-
-	init_completion(&ar->offchan_tx_completed);
-	INIT_WORK(&ar->offchan_tx_work, ath10k_offchan_tx_work);
-	skb_queue_head_init(&ar->offchan_tx_queue);
-
-	INIT_WORK(&ar->wmi_mgmt_tx_work, ath10k_mgmt_over_wmi_tx_work);
-	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
-
-	INIT_WORK(&ar->restart_work, ath10k_core_restart);
-
-	return ar;
-
-err_wq:
-	ath10k_mac_destroy(ar);
-	return NULL;
-}
-EXPORT_SYMBOL(ath10k_core_create);
-
-void ath10k_core_destroy(struct ath10k *ar)
-{
-	flush_workqueue(ar->workqueue);
-	destroy_workqueue(ar->workqueue);
-
-	ath10k_mac_destroy(ar);
-}
-EXPORT_SYMBOL(ath10k_core_destroy);
-
 int ath10k_core_start(struct ath10k *ar)
 {
 	int status;
@@ -1058,6 +994,70 @@ void ath10k_core_unregister(struct ath10k *ar)
 }
 EXPORT_SYMBOL(ath10k_core_unregister);
 
+struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
+				  const struct ath10k_hif_ops *hif_ops)
+{
+	struct ath10k *ar;
+
+	ar = ath10k_mac_create();
+	if (!ar)
+		return NULL;
+
+	ar->ath_common.priv = ar;
+	ar->ath_common.hw = ar->hw;
+
+	ar->p2p = !!ath10k_p2p;
+	ar->dev = dev;
+
+	ar->hif.priv = hif_priv;
+	ar->hif.ops = hif_ops;
+
+	init_completion(&ar->scan.started);
+	init_completion(&ar->scan.completed);
+	init_completion(&ar->scan.on_channel);
+	init_completion(&ar->target_suspend);
+
+	init_completion(&ar->install_key_done);
+	init_completion(&ar->vdev_setup_done);
+
+	setup_timer(&ar->scan.timeout, ath10k_reset_scan, (unsigned long)ar);
+
+	ar->workqueue = create_singlethread_workqueue("ath10k_wq");
+	if (!ar->workqueue)
+		goto err_wq;
+
+	mutex_init(&ar->conf_mutex);
+	spin_lock_init(&ar->data_lock);
+
+	INIT_LIST_HEAD(&ar->peers);
+	init_waitqueue_head(&ar->peer_mapping_wq);
+
+	init_completion(&ar->offchan_tx_completed);
+	INIT_WORK(&ar->offchan_tx_work, ath10k_offchan_tx_work);
+	skb_queue_head_init(&ar->offchan_tx_queue);
+
+	INIT_WORK(&ar->wmi_mgmt_tx_work, ath10k_mgmt_over_wmi_tx_work);
+	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
+
+	INIT_WORK(&ar->restart_work, ath10k_core_restart);
+
+	return ar;
+
+err_wq:
+	ath10k_mac_destroy(ar);
+	return NULL;
+}
+EXPORT_SYMBOL(ath10k_core_create);
+
+void ath10k_core_destroy(struct ath10k *ar)
+{
+	flush_workqueue(ar->workqueue);
+	destroy_workqueue(ar->workqueue);
+
+	ath10k_mac_destroy(ar);
+}
+EXPORT_SYMBOL(ath10k_core_destroy);
+
 MODULE_AUTHOR("Qualcomm Atheros");
 MODULE_DESCRIPTION("Core module for QCA988X PCIe devices.");
 MODULE_LICENSE("Dual BSD/GPL");
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

If ath10k was built into the kernel it could stall
booting for 120 seconds by default (60 seconds for
each firmware API variant) waiting for firmware
files before userspace was ready or filesystems
mounted.

Fix this by making the core registering
asynchronous.

This also shoves off about 1 second from boot time
on most systems since the driver is now mostly
initialized in a worker and modprobe takes very
little time to complete.

As a side effect there's no way to propagate
registering errors to the pci subsystem but this
probably isn't really necessary.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 42 ++++++++++++++++++++++++----------
 drivers/net/wireless/ath/ath10k/core.h |  2 ++
 drivers/net/wireless/ath/ath10k/pci.c  |  2 --
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index bd4d1cc..d20ac66 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -941,22 +941,15 @@ static int ath10k_core_check_chip_id(struct ath10k *ar)
 	return 0;
 }
 
-int ath10k_core_register(struct ath10k *ar, u32 chip_id)
+void ath10k_core_register_work(struct work_struct *work)
 {
+	struct ath10k *ar = container_of(work, struct ath10k, register_work);
 	int status;
 
-	ar->chip_id = chip_id;
-
-	status = ath10k_core_check_chip_id(ar);
-	if (status) {
-		ath10k_err("Unsupported chip id 0x%08x\n", ar->chip_id);
-		return status;
-	}
-
 	status = ath10k_core_probe_fw(ar);
 	if (status) {
 		ath10k_err("could not probe fw (%d)\n", status);
-		return status;
+		goto err;
 	}
 
 	status = ath10k_mac_register(ar);
@@ -971,18 +964,42 @@ int ath10k_core_register(struct ath10k *ar, u32 chip_id)
 		goto err_unregister_mac;
 	}
 
-	return 0;
+	set_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags);
+	return;
 
 err_unregister_mac:
 	ath10k_mac_unregister(ar);
 err_release_fw:
 	ath10k_core_free_firmware_files(ar);
-	return status;
+err:
+	return;
+}
+
+int ath10k_core_register(struct ath10k *ar, u32 chip_id)
+{
+	int status;
+
+	ar->chip_id = chip_id;
+
+	status = ath10k_core_check_chip_id(ar);
+	if (status) {
+		ath10k_err("Unsupported chip id 0x%08x\n", ar->chip_id);
+		return status;
+	}
+
+	queue_work(ar->workqueue, &ar->register_work);
+
+	return 0;
 }
 EXPORT_SYMBOL(ath10k_core_register);
 
 void ath10k_core_unregister(struct ath10k *ar)
 {
+	cancel_work_sync(&ar->register_work);
+
+	if (!test_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags))
+		return;
+
 	/* We must unregister from mac80211 before we stop HTC and HIF.
 	 * Otherwise we will fail to submit commands to FW and mac80211 will be
 	 * unhappy about callback failures. */
@@ -1039,6 +1056,7 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 	INIT_WORK(&ar->wmi_mgmt_tx_work, ath10k_mgmt_over_wmi_tx_work);
 	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
 
+	INIT_WORK(&ar->register_work, ath10k_core_register_work);
 	INIT_WORK(&ar->restart_work, ath10k_core_restart);
 
 	return ar;
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 2c1dfd7..7e33005 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -335,6 +335,7 @@ enum ath10k_dev_flags {
 	/* Indicates that ath10k device is during CAC phase of DFS */
 	ATH10K_CAC_RUNNING,
 	ATH10K_FLAG_FIRST_BOOT_DONE,
+	ATH10K_FLAG_CORE_REGISTERED,
 };
 
 struct ath10k {
@@ -470,6 +471,7 @@ struct ath10k {
 
 	enum ath10k_state state;
 
+	struct work_struct register_work;
 	struct work_struct restart_work;
 
 	/* cycle count is reported twice for each visited channel during scan.
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7d72b9c..dadf4ce 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2758,8 +2758,6 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
 	if (!ar_pci)
 		return;
 
-	tasklet_kill(&ar_pci->msi_fw_err);
-
 	ath10k_core_unregister(ar);
 	ath10k_pci_free_ce(ar);
 
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Johannes Berg]

On Thu, 2014-05-22 at 16:12 +0200, Michal Kazior wrote:

> As a side effect there's no way to propagate
> registering errors to the pci subsystem but this
> probably isn't really necessary.

You should probably unbind if it fails - iwlwifi does that.

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

On 22 May 2014 16:41, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-05-22 at 16:12 +0200, Michal Kazior wrote:
>
>> As a side effect there's no way to propagate
>> registering errors to the pci subsystem but this
>> probably isn't really necessary.
>
> You should probably unbind if it fails - iwlwifi does that.

You mean device_release_driver()? I didn't know about it. Thanks!


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

Hi Johannes,


On 22 May 2014 16:41, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Thu, 2014-05-22 at 16:12 +0200, Michal Kazior wrote:
>
>> As a side effect there's no way to propagate
>> registering errors to the pci subsystem but this
>> probably isn't really necessary.
>
> You should probably unbind if it fails - iwlwifi does that.

Hmm..

As it stands ath10k now deadlocks when firmware probing worker fails
and calls device_release_driver(). This indirectly hits pci remove()
callback which waits for the worker. Oops.

So I've taken a look how other driver do it so I can come up with a
solution - most of them seem use completions. After taking a closer
look I came to a conclusion this is inherently racy too. Consider the
following scenario:

 [syscall]
 insmod
 pci->probe()
 queue_work()
 return
 [worker]
 probe_fw()
 [syscall]
 rmmod
 dev_lock()
 pci->remove()
 wait_for_completion()
 [worker]
 complete_all()
 device_release_driver()
 dev_lock()
 {already held, yield}
 [syscall]
 free(internal structures)
 dev_unlock()
 return
 [worker]
 {woken up, but dev->driver == NULL so no callbacks}
 dev_unlock()
 return

The driver code section may not be reachable anymore upon worker
returning from the device_release_driver() call, right? Also since
ath10k uses an internal worker it also means the work_struct would be
already freed by the syscall flow (i.e. worker would run after driver
has supposedly been cleaned up..). Even if ath10k was to use
request_firmware_nowait(), which allocates a temporary work_struct,
the unreachable code section still remains a problem.

Or maybe this isn't really a problem and/or I'm missing something?


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Johannes Berg]

Hi,

>  [worker]
>  complete_all()
>  device_release_driver()
>  dev_lock()
>  {already held, yield}
>  [syscall]
>  free(internal structures)
>  dev_unlock()
>  return
>  [worker]
>  {woken up, but dev->driver == NULL so no callbacks}
>  dev_unlock()
>  return
> 
> The driver code section may not be reachable anymore upon worker
> returning from the device_release_driver() call, right? Also since
> ath10k uses an internal worker it also means the work_struct would be
> already freed by the syscall flow (i.e. worker would run after driver
> has supposedly been cleaned up..). Even if ath10k was to use
> request_firmware_nowait(), which allocates a temporary work_struct,
> the unreachable code section still remains a problem.
> 
> Or maybe this isn't really a problem and/or I'm missing something?

Yeah, hmm, this looks like a problem. I guess we didn't really consider
module unload in such detail ...

I guess this would crash upon return from device_release_driver()? I
guess if that's the last thing then maybe we'd actually get a tail-call
optimisation, but we don't want to rely on that of course!

Seems like to fix it we just need to get a module reference though? Can
a module put() itself though? Hmmm.

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

On 23 June 2014 13:51, Johannes Berg <johannes@sipsolutions.net> wrote:
> Hi,
>
>>  [worker]
>>  complete_all()
>>  device_release_driver()
>>  dev_lock()
>>  {already held, yield}
>>  [syscall]
>>  free(internal structures)
>>  dev_unlock()
>>  return
>>  [worker]
>>  {woken up, but dev->driver == NULL so no callbacks}
>>  dev_unlock()
>>  return
>>
>> The driver code section may not be reachable anymore upon worker
>> returning from the device_release_driver() call, right? Also since
>> ath10k uses an internal worker it also means the work_struct would be
>> already freed by the syscall flow (i.e. worker would run after driver
>> has supposedly been cleaned up..). Even if ath10k was to use
>> request_firmware_nowait(), which allocates a temporary work_struct,
>> the unreachable code section still remains a problem.
>>
>> Or maybe this isn't really a problem and/or I'm missing something?
>
> Yeah, hmm, this looks like a problem. I guess we didn't really consider
> module unload in such detail ...
>
> I guess this would crash upon return from device_release_driver()? I
> guess if that's the last thing then maybe we'd actually get a tail-call
> optimisation, but we don't want to rely on that of course!
>
> Seems like to fix it we just need to get a module reference though? Can
> a module put() itself though? Hmmm.

It seems some drivers use module_put(THIS_MODULE) and
__module_get(THIS_MODULE), e.g. tun/tap driver.

But does this bump up the module refcount in such a way that an
in-progress rmmod will wait/block until the refcount reaches 0?


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Johannes Berg]

On Mon, 2014-06-23 at 14:17 +0200, Michal Kazior wrote:

> > Seems like to fix it we just need to get a module reference though? Can
> > a module put() itself though? Hmmm.
> 
> It seems some drivers use module_put(THIS_MODULE) and
> __module_get(THIS_MODULE), e.g. tun/tap driver.

I don't see how this isn't racy in similar ways though?

> But does this bump up the module refcount in such a way that an
> in-progress rmmod will wait/block until the refcount reaches 0?

It should, yes, but you race once you do module_put(), no?

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

On 23 June 2014 14:32, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2014-06-23 at 14:17 +0200, Michal Kazior wrote:
>
>> > Seems like to fix it we just need to get a module reference though? Can
>> > a module put() itself though? Hmmm.
>>
>> It seems some drivers use module_put(THIS_MODULE) and
>> __module_get(THIS_MODULE), e.g. tun/tap driver.
>
> I don't see how this isn't racy in similar ways though?

I think tun/tap uses it for different purposes.


>> But does this bump up the module refcount in such a way that an
>> in-progress rmmod will wait/block until the refcount reaches 0?
>
> It should, yes, but you race once you do module_put(), no?

It's still racy, I agree.

I don't see how device_release_driver() can be made work synchronously
at all. It should, however, be perfectly fine if this was an async
request to release a device/driver, right?


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Johannes Berg]

On Mon, 2014-06-23 at 14:59 +0200, Michal Kazior wrote:
> >> But does this bump up the module refcount in such a way that an
> >> in-progress rmmod will wait/block until the refcount reaches 0?
> >
> > It should, yes, but you race once you do module_put(), no?
> 
> It's still racy, I agree.
> 
> I don't see how device_release_driver() can be made work synchronously
> at all. It should, however, be perfectly fine if this was an async
> request to release a device/driver, right?

Yeah, but the work struct would have to live somewhere that can't be
unloaded, or can more safely wait for it...

I guess we could also have a list of these in the modules and
synchronize on them in module_exit()?

johannes


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> If ath10k was built into the kernel it could stall
> booting for 120 seconds by default (60 seconds for
> each firmware API variant) waiting for firmware
> files before userspace was ready or filesystems
> mounted.
>
> Fix this by making the core registering
> asynchronous.
>
> This also shoves off about 1 second from boot time
> on most systems since the driver is now mostly
> initialized in a worker and modprobe takes very
> little time to complete.
>
> As a side effect there's no way to propagate
> registering errors to the pci subsystem but this
> probably isn't really necessary.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

I see a new sparse warning with this patch:

drivers/net/wireless/ath/ath10k/core.c:956:6: warning: symbol 'ath10k_core_register_work' was not declared. Should it be static?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

On 22 May 2014 18:40, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> If ath10k was built into the kernel it could stall
>> booting for 120 seconds by default (60 seconds for
>> each firmware API variant) waiting for firmware
>> files before userspace was ready or filesystems
>> mounted.
>>
>> Fix this by making the core registering
>> asynchronous.
>>
>> This also shoves off about 1 second from boot time
>> on most systems since the driver is now mostly
>> initialized in a worker and modprobe takes very
>> little time to complete.
>>
>> As a side effect there's no way to propagate
>> registering errors to the pci subsystem but this
>> probably isn't really necessary.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> I see a new sparse warning with this patch:
>
> drivers/net/wireless/ath/ath10k/core.c:956:6: warning: symbol 'ath10k_core_register_work' was not declared. Should it be static?

Yeah. I forgot to add static keyword since I've played a little around
the code ealier. I'll fix it, thanks!


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> If ath10k was built into the kernel it could stall
> booting for 120 seconds by default (60 seconds for
> each firmware API variant) waiting for firmware
> files before userspace was ready or filesystems
> mounted.
>
> Fix this by making the core registering
> asynchronous.
>
> This also shoves off about 1 second from boot time
> on most systems since the driver is now mostly
> initialized in a worker and modprobe takes very
> little time to complete.
>
> As a side effect there's no way to propagate
> registering errors to the pci subsystem but this
> probably isn't really necessary.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/pci.c
> +++ b/drivers/net/wireless/ath/ath10k/pci.c
> @@ -2758,8 +2758,6 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
>  	if (!ar_pci)
>  		return;
>  
> -	tasklet_kill(&ar_pci->msi_fw_err);

Why this? That's not obvious to me.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Michal Kazior]

On 22 May 2014 18:42, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> If ath10k was built into the kernel it could stall
>> booting for 120 seconds by default (60 seconds for
>> each firmware API variant) waiting for firmware
>> files before userspace was ready or filesystems
>> mounted.
>>
>> Fix this by making the core registering
>> asynchronous.
>>
>> This also shoves off about 1 second from boot time
>> on most systems since the driver is now mostly
>> initialized in a worker and modprobe takes very
>> little time to complete.
>>
>> As a side effect there's no way to propagate
>> registering errors to the pci subsystem but this
>> probably isn't really necessary.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -2758,8 +2758,6 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
>>       if (!ar_pci)
>>               return;
>>
>> -     tasklet_kill(&ar_pci->msi_fw_err);
>
> Why this? That's not obvious to me.

It might end up not being initialized: If you, say, quickly insert and
eject a device you might not even initialize the tasklet (it's done
elsewhere; I suppose we could move it too) before ath10k_pci_remove()
is called. Also the tasklet is already killed elsewhere on the
teardown code path.

I'll split it into a separate patch that simply removes this line as
it's meaningless to call it here in the first place.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 2/2] ath10k: make core registering async

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

>>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>>> @@ -2758,8 +2758,6 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
>>>       if (!ar_pci)
>>>               return;
>>>
>>> -     tasklet_kill(&ar_pci->msi_fw_err);
>>
>> Why this? That's not obvious to me.
>
> It might end up not being initialized: If you, say, quickly insert and
> eject a device you might not even initialize the tasklet (it's done
> elsewhere; I suppose we could move it too) before ath10k_pci_remove()
> is called. Also the tasklet is already killed elsewhere on the
> teardown code path.
>
> I'll split it into a separate patch that simply removes this line as
> it's meaningless to call it here in the first place.

Yeah, it's a good idea to have a separate patch for this.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 0/3] ath10k: async pci probing

[Michal Kazior]

Hi,

Recently I've tried to build ath10k into kernel
and discovered it doesn't play nice.


v2:
 * split and introduce an additional patch [2/3]
 * unbind driver upon failure


Michal Kazior (3):
  ath10k: relocate core create/destroy functions
  ath10k: remove unnecessary tasklet_kill()
  ath10k: make core registering async

 drivers/net/wireless/ath/ath10k/core.c | 171 ++++++++++++++++++---------------
 drivers/net/wireless/ath/ath10k/core.h |   2 +
 drivers/net/wireless/ath/ath10k/pci.c  |   2 -
 3 files changed, 97 insertions(+), 78 deletions(-)

-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 1/3] ath10k: relocate core create/destroy functions

[Michal Kazior]

This will avoid unnecessary forward declaration of
any kind in the future.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 128 ++++++++++++++++-----------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 75b3dfb..bd4d1cc 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -701,70 +701,6 @@ static void ath10k_core_restart(struct work_struct *work)
 	mutex_unlock(&ar->conf_mutex);
 }
 
-struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
-				  const struct ath10k_hif_ops *hif_ops)
-{
-	struct ath10k *ar;
-
-	ar = ath10k_mac_create();
-	if (!ar)
-		return NULL;
-
-	ar->ath_common.priv = ar;
-	ar->ath_common.hw = ar->hw;
-
-	ar->p2p = !!ath10k_p2p;
-	ar->dev = dev;
-
-	ar->hif.priv = hif_priv;
-	ar->hif.ops = hif_ops;
-
-	init_completion(&ar->scan.started);
-	init_completion(&ar->scan.completed);
-	init_completion(&ar->scan.on_channel);
-	init_completion(&ar->target_suspend);
-
-	init_completion(&ar->install_key_done);
-	init_completion(&ar->vdev_setup_done);
-
-	setup_timer(&ar->scan.timeout, ath10k_reset_scan, (unsigned long)ar);
-
-	ar->workqueue = create_singlethread_workqueue("ath10k_wq");
-	if (!ar->workqueue)
-		goto err_wq;
-
-	mutex_init(&ar->conf_mutex);
-	spin_lock_init(&ar->data_lock);
-
-	INIT_LIST_HEAD(&ar->peers);
-	init_waitqueue_head(&ar->peer_mapping_wq);
-
-	init_completion(&ar->offchan_tx_completed);
-	INIT_WORK(&ar->offchan_tx_work, ath10k_offchan_tx_work);
-	skb_queue_head_init(&ar->offchan_tx_queue);
-
-	INIT_WORK(&ar->wmi_mgmt_tx_work, ath10k_mgmt_over_wmi_tx_work);
-	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
-
-	INIT_WORK(&ar->restart_work, ath10k_core_restart);
-
-	return ar;
-
-err_wq:
-	ath10k_mac_destroy(ar);
-	return NULL;
-}
-EXPORT_SYMBOL(ath10k_core_create);
-
-void ath10k_core_destroy(struct ath10k *ar)
-{
-	flush_workqueue(ar->workqueue);
-	destroy_workqueue(ar->workqueue);
-
-	ath10k_mac_destroy(ar);
-}
-EXPORT_SYMBOL(ath10k_core_destroy);
-
 int ath10k_core_start(struct ath10k *ar)
 {
 	int status;
@@ -1058,6 +994,70 @@ void ath10k_core_unregister(struct ath10k *ar)
 }
 EXPORT_SYMBOL(ath10k_core_unregister);
 
+struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
+				  const struct ath10k_hif_ops *hif_ops)
+{
+	struct ath10k *ar;
+
+	ar = ath10k_mac_create();
+	if (!ar)
+		return NULL;
+
+	ar->ath_common.priv = ar;
+	ar->ath_common.hw = ar->hw;
+
+	ar->p2p = !!ath10k_p2p;
+	ar->dev = dev;
+
+	ar->hif.priv = hif_priv;
+	ar->hif.ops = hif_ops;
+
+	init_completion(&ar->scan.started);
+	init_completion(&ar->scan.completed);
+	init_completion(&ar->scan.on_channel);
+	init_completion(&ar->target_suspend);
+
+	init_completion(&ar->install_key_done);
+	init_completion(&ar->vdev_setup_done);
+
+	setup_timer(&ar->scan.timeout, ath10k_reset_scan, (unsigned long)ar);
+
+	ar->workqueue = create_singlethread_workqueue("ath10k_wq");
+	if (!ar->workqueue)
+		goto err_wq;
+
+	mutex_init(&ar->conf_mutex);
+	spin_lock_init(&ar->data_lock);
+
+	INIT_LIST_HEAD(&ar->peers);
+	init_waitqueue_head(&ar->peer_mapping_wq);
+
+	init_completion(&ar->offchan_tx_completed);
+	INIT_WORK(&ar->offchan_tx_work, ath10k_offchan_tx_work);
+	skb_queue_head_init(&ar->offchan_tx_queue);
+
+	INIT_WORK(&ar->wmi_mgmt_tx_work, ath10k_mgmt_over_wmi_tx_work);
+	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
+
+	INIT_WORK(&ar->restart_work, ath10k_core_restart);
+
+	return ar;
+
+err_wq:
+	ath10k_mac_destroy(ar);
+	return NULL;
+}
+EXPORT_SYMBOL(ath10k_core_create);
+
+void ath10k_core_destroy(struct ath10k *ar)
+{
+	flush_workqueue(ar->workqueue);
+	destroy_workqueue(ar->workqueue);
+
+	ath10k_mac_destroy(ar);
+}
+EXPORT_SYMBOL(ath10k_core_destroy);
+
 MODULE_AUTHOR("Qualcomm Atheros");
 MODULE_DESCRIPTION("Core module for QCA988X PCIe devices.");
 MODULE_LICENSE("Dual BSD/GPL");
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 2/3] ath10k: remove unnecessary tasklet_kill()

[Michal Kazior]

The tasklet is already guaranteed to be killed on
the teardown path.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

Notes:
    v2:
     * split from another patch [Kalle]

 drivers/net/wireless/ath/ath10k/pci.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
index 7d72b9c..dadf4ce 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -2758,8 +2758,6 @@ static void ath10k_pci_remove(struct pci_dev *pdev)
 	if (!ar_pci)
 		return;
 
-	tasklet_kill(&ar_pci->msi_fw_err);
-
 	ath10k_core_unregister(ar);
 	ath10k_pci_free_ce(ar);
 
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 3/3] ath10k: make core registering async

[Michal Kazior]

If ath10k was built into the kernel it could stall
booting for 120 seconds by default (60 seconds for
each firmware API variant) waiting for firmware
files before userspace was ready or filesystems
mounted.

Fix this by making the core registering
asynchronous.

This also shoves off about 1 second from boot time
on most systems since the driver is now mostly
initialized in a worker and modprobe takes very
little time to complete.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---

Notes:
    v2:
     * use device_release_driver() [Johannes]
     * update commit message (due to above)
     * split tasklet_kill() removal to a separate patch [Kalle]
     * add missing static keyword to worker function [Kalle]

 drivers/net/wireless/ath/ath10k/core.c | 43 ++++++++++++++++++++++++----------
 drivers/net/wireless/ath/ath10k/core.h |  2 ++
 2 files changed, 33 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index bd4d1cc..f922ef0 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -941,22 +941,15 @@ static int ath10k_core_check_chip_id(struct ath10k *ar)
 	return 0;
 }
 
-int ath10k_core_register(struct ath10k *ar, u32 chip_id)
+static void ath10k_core_register_work(struct work_struct *work)
 {
+	struct ath10k *ar = container_of(work, struct ath10k, register_work);
 	int status;
 
-	ar->chip_id = chip_id;
-
-	status = ath10k_core_check_chip_id(ar);
-	if (status) {
-		ath10k_err("Unsupported chip id 0x%08x\n", ar->chip_id);
-		return status;
-	}
-
 	status = ath10k_core_probe_fw(ar);
 	if (status) {
 		ath10k_err("could not probe fw (%d)\n", status);
-		return status;
+		goto err;
 	}
 
 	status = ath10k_mac_register(ar);
@@ -971,18 +964,43 @@ int ath10k_core_register(struct ath10k *ar, u32 chip_id)
 		goto err_unregister_mac;
 	}
 
-	return 0;
+	set_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags);
+	return;
 
 err_unregister_mac:
 	ath10k_mac_unregister(ar);
 err_release_fw:
 	ath10k_core_free_firmware_files(ar);
-	return status;
+err:
+	device_release_driver(ar->dev);
+	return;
+}
+
+int ath10k_core_register(struct ath10k *ar, u32 chip_id)
+{
+	int status;
+
+	ar->chip_id = chip_id;
+
+	status = ath10k_core_check_chip_id(ar);
+	if (status) {
+		ath10k_err("Unsupported chip id 0x%08x\n", ar->chip_id);
+		return status;
+	}
+
+	queue_work(ar->workqueue, &ar->register_work);
+
+	return 0;
 }
 EXPORT_SYMBOL(ath10k_core_register);
 
 void ath10k_core_unregister(struct ath10k *ar)
 {
+	cancel_work_sync(&ar->register_work);
+
+	if (!test_bit(ATH10K_FLAG_CORE_REGISTERED, &ar->dev_flags))
+		return;
+
 	/* We must unregister from mac80211 before we stop HTC and HIF.
 	 * Otherwise we will fail to submit commands to FW and mac80211 will be
 	 * unhappy about callback failures. */
@@ -1039,6 +1057,7 @@ struct ath10k *ath10k_core_create(void *hif_priv, struct device *dev,
 	INIT_WORK(&ar->wmi_mgmt_tx_work, ath10k_mgmt_over_wmi_tx_work);
 	skb_queue_head_init(&ar->wmi_mgmt_tx_queue);
 
+	INIT_WORK(&ar->register_work, ath10k_core_register_work);
 	INIT_WORK(&ar->restart_work, ath10k_core_restart);
 
 	return ar;
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 2c1dfd7..7e33005 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -335,6 +335,7 @@ enum ath10k_dev_flags {
 	/* Indicates that ath10k device is during CAC phase of DFS */
 	ATH10K_CAC_RUNNING,
 	ATH10K_FLAG_FIRST_BOOT_DONE,
+	ATH10K_FLAG_CORE_REGISTERED,
 };
 
 struct ath10k {
@@ -470,6 +471,7 @@ struct ath10k {
 
 	enum ath10k_state state;
 
+	struct work_struct register_work;
 	struct work_struct restart_work;
 
 	/* cycle count is reported twice for each visited channel during scan.
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 0/3] ath10k: async pci probing

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Hi,
>
> Recently I've tried to build ath10k into kernel
> and discovered it doesn't play nice.
>
>
> v2:
>  * split and introduce an additional patch [2/3]
>  * unbind driver upon failure
>
>
> Michal Kazior (3):
>   ath10k: relocate core create/destroy functions
>   ath10k: remove unnecessary tasklet_kill()
>   ath10k: make core registering async

Thanks, all three applied.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k