On 2022/06/11 7:51, Tetsuo Handa wrote:
> On 2022/06/11 4:10, Jeff Johnson wrote:
>> On 6/10/2022 12:05 PM, Jeff Johnson wrote:
>>>> +static int __init ath6kl_init(void)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ath6kl_wq = alloc_workqueue("ath6kl_wq", 0, 0);
>>>> + if (!ath6kl_wq)
>>>> + return -ENOMEM;
>>>
>>> this approach means the driver will always allocate a workqueue even if the associated hardware is never present.
>
> Creating a WQ without WQ_MEM_RECLAIM flag consumes little resource.
My understanding is that a loadable kernel module file is modprobe'd when
the kernel detected a hardware and some userspace program determined a .ko
file to use based on device IDs database). Thus, I think it is very likely
that an associated hardware presents if module's __init function is called.
If a module is built-in, that user is ready to tolerate memory footprint
wasted by non-present hardware. And I think memory wasted by keeping an
unused !WQ_MEM_RECLAIM workqueue is much smaller than memory wasted by
keeping that module.
Thus, I wonder who complains about creating possibly unused !WQ_MEM_RECLAIM
workqueue, unless that module is designed for tiny embedded environments.
>
>>>
>>> did you consider instead having the allocation take place within the processing of ath6kl_usb_probe() and the destroy take place within the processing of ath6kl_usb_pm_remove()?
>>
>> typo: ath6kl_usb_pm_remove() => ath6kl_usb_remove()
>
> Technically possible to use ath6kl_usb_create()/ath6kl_usb_destroy() if you prefer it.
>
> Do you want ath6kl_wq be shared within this module (using a refcount), or be local to
> each "struct ath6kl_usb" (adding a member and accessing via usb_get_intfdata()) ?
>
Anyway, here goes per "struct ath6kl_usb" version.
>From 00c560307d72abffea29409328be8cd69abecc95 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
Date: Sun, 12 Jun 2022 10:38:03 +0900
Subject: [PATCH v2] ath6kl: avoid flush_scheduled_work() usage
Use local wq in order to avoid flush_scheduled_work() usage.
Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
---
Changes in v2:
Use per "struct ath6kl_usb" workqueue instead of per module workqueue.
Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue()
using a macro") for background.
This is a blind conversion, and is only compile tested.
drivers/net/wireless/ath/ath6kl/usb.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/usb.c b/drivers/net/wireless/ath/ath6kl/usb.c
index 65e683effdcb..5220809841a6 100644
--- a/drivers/net/wireless/ath/ath6kl/usb.c
+++ b/drivers/net/wireless/ath/ath6kl/usb.c
@@ -71,6 +71,7 @@ struct ath6kl_usb {
u8 *diag_cmd_buffer;
u8 *diag_resp_buffer;
struct ath6kl *ar;
+ struct workqueue_struct *wq;
};
/* usb urb object */
@@ -478,7 +479,7 @@ static void ath6kl_usb_flush_all(struct ath6kl_usb *ar_usb)
* Flushing any pending I/O may schedule work this call will block
* until all scheduled work runs to completion.
*/
- flush_scheduled_work();
+ flush_workqueue(ar_usb->wq);
}
static void ath6kl_usb_start_recv_pipes(struct ath6kl_usb *ar_usb)
@@ -544,7 +545,7 @@ static void ath6kl_usb_recv_complete(struct urb *urb)
/* note: queue implements a lock */
skb_queue_tail(&pipe->io_comp_queue, skb);
- schedule_work(&pipe->io_complete_work);
+ queue_work(pipe->ar_usb->wq, &pipe->io_complete_work);
cleanup_recv_urb:
ath6kl_usb_cleanup_recv_urb(urb_context);
@@ -579,7 +580,7 @@ static void ath6kl_usb_usb_transmit_complete(struct urb *urb)
/* note: queue implements a lock */
skb_queue_tail(&pipe->io_comp_queue, skb);
- schedule_work(&pipe->io_complete_work);
+ queue_work(pipe->ar_usb->wq, &pipe->io_complete_work);
}
static void ath6kl_usb_io_comp_work(struct work_struct *work)
@@ -619,6 +620,7 @@ static void ath6kl_usb_destroy(struct ath6kl_usb *ar_usb)
kfree(ar_usb->diag_cmd_buffer);
kfree(ar_usb->diag_resp_buffer);
+ destroy_workqueue(ar_usb->wq);
kfree(ar_usb);
}
@@ -631,9 +633,15 @@ static struct ath6kl_usb *ath6kl_usb_create(struct usb_interface *interface)
int status = 0;
int i;
+ /* ath6kl_usb_destroy() needs ar_usb != NULL && ar_usb->wq != NULL. */
ar_usb = kzalloc(sizeof(struct ath6kl_usb), GFP_KERNEL);
if (ar_usb == NULL)
- goto fail_ath6kl_usb_create;
+ return NULL;
+ ar_usb->wq = alloc_workqueue("ath6kl_wq", 0, 0);
+ if (!ar_usb->wq) {
+ kfree(ar_usb);
+ return NULL;
+ }
usb_set_intfdata(interface, ar_usb);
spin_lock_init(&(ar_usb->cs_lock));
--
2.18.4
