[PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[Amitoj Kaur Chawla]

With concurrency managed workqueues, use of dedicated workqueues
can be replaced by using system_wq. Drop usb_tx_wq and usb_rx_wq by 
using system_wq.

Since there are multiple work items per udev but different udevs
do not need to be ordered, increase of concurrency level by switching
to system_wq should not break anything.

cancel_work_sync() is used to ensure that work is not pending or
executing on any CPU.

Lastly, since all devices are suspended, which shutdowns the work item
before the driver can be unregistered, it is guaranteed that no work
item is pending or executing by the time exit path runs.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/gdm724x/gdm_usb.c | 33 +++++++--------------------------
 1 file changed, 7 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
index 92ea1a1..beef701 100644
--- a/drivers/staging/gdm724x/gdm_usb.c
+++ b/drivers/staging/gdm724x/gdm_usb.c
@@ -55,9 +55,6 @@ static const struct usb_device_id id_table[] = {
 
 MODULE_DEVICE_TABLE(usb, id_table);
 
-static struct workqueue_struct *usb_tx_wq;
-static struct workqueue_struct *usb_rx_wq;
-
 static void do_tx(struct work_struct *work);
 static void do_rx(struct work_struct *work);
 
@@ -476,7 +473,7 @@ static void gdm_usb_rcv_complete(struct urb *urb)
 	if (!urb->status && r->callback) {
 		spin_lock_irqsave(&rx->to_host_lock, flags);
 		list_add_tail(&r->to_host_list, &rx->to_host_list);
-		queue_work(usb_rx_wq, &udev->work_rx.work);
+		schedule_work(&udev->work_rx.work);
 		spin_unlock_irqrestore(&rx->to_host_lock, flags);
 	} else {
 		if (urb->status && udev->usb_state == PM_NORMAL)
@@ -568,7 +565,7 @@ static void gdm_usb_send_complete(struct urb *urb)
 
 	spin_lock_irqsave(&tx->lock, flags);
 	udev->send_complete = 1;
-	queue_work(usb_tx_wq, &udev->work_tx.work);
+	schedule_work(&udev->work_tx.work);
 	spin_unlock_irqrestore(&tx->lock, flags);
 }
 
@@ -759,7 +756,7 @@ static int gdm_usb_sdu_send(void *priv_dev, void *data, int len,
 
 	spin_lock_irqsave(&tx->lock, flags);
 	list_add_tail(&t_sdu->list, &tx->sdu_list);
-	queue_work(usb_tx_wq, &udev->work_tx.work);
+	schedule_work(&udev->work_tx.work);
 	spin_unlock_irqrestore(&tx->lock, flags);
 
 	if (no_spc)
@@ -796,7 +793,7 @@ static int gdm_usb_hci_send(void *priv_dev, void *data, int len,
 
 	spin_lock_irqsave(&tx->lock, flags);
 	list_add_tail(&t->list, &tx->hci_list);
-	queue_work(usb_tx_wq, &udev->work_tx.work);
+	schedule_work(&udev->work_tx.work);
 	spin_unlock_irqrestore(&tx->lock, flags);
 
 	return 0;
@@ -944,6 +941,8 @@ static int gdm_usb_suspend(struct usb_interface *intf, pm_message_t pm_msg)
 	}
 	spin_unlock_irqrestore(&rx->submit_lock, flags);
 
+	cancel_work_sync(&udev->work_tx.work);
+
 	return 0;
 }
 
@@ -981,7 +980,7 @@ static int gdm_usb_resume(struct usb_interface *intf)
 
 	tx = &udev->tx;
 	spin_lock_irqsave(&tx->lock, flags);
-	queue_work(usb_tx_wq, &udev->work_tx.work);
+	schedule_work(&udev->work_tx.work);
 	spin_unlock_irqrestore(&tx->lock, flags);
 
 	return 0;
@@ -1005,14 +1004,6 @@ static int __init gdm_usb_lte_init(void)
 		return -1;
 	}
 
-	usb_tx_wq = create_workqueue("usb_tx_wq");
-	if (!usb_tx_wq)
-		return -1;
-
-	usb_rx_wq = create_workqueue("usb_rx_wq");
-	if (!usb_rx_wq)
-		return -1;
-
 	return usb_register(&gdm_usb_lte_driver);
 }
 
@@ -1021,16 +1012,6 @@ static void __exit gdm_usb_lte_exit(void)
 	gdm_lte_event_exit();
 
 	usb_deregister(&gdm_usb_lte_driver);
-
-	if (usb_tx_wq) {
-		flush_workqueue(usb_tx_wq);
-		destroy_workqueue(usb_tx_wq);
-	}
-
-	if (usb_rx_wq) {
-		flush_workqueue(usb_rx_wq);
-		destroy_workqueue(usb_rx_wq);
-	}
 }
 
 module_init(gdm_usb_lte_init);
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[Tejun Heo]

On Thu, Feb 25, 2016 at 08:49:00AM +0530, Amitoj Kaur Chawla wrote:
> With concurrency managed workqueues, use of dedicated workqueues
> can be replaced by using system_wq. Drop usb_tx_wq and usb_rx_wq by 
> using system_wq.
> 
> Since there are multiple work items per udev but different udevs
> do not need to be ordered, increase of concurrency level by switching
> to system_wq should not break anything.
> 
> cancel_work_sync() is used to ensure that work is not pending or
> executing on any CPU.
> 
> Lastly, since all devices are suspended, which shutdowns the work item
> before the driver can be unregistered, it is guaranteed that no work
> item is pending or executing by the time exit path runs.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>

Excellent work.  It'd be great if the somebody can test the changes on
the actual hardware but for the workqueue part.

Acked-by: Tejun Heo <tj@kernel.org>

Thank a lot!

-- 
tejun

Re: [Outreachy kernel] [PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[Amitoj Kaur Chawla]

On Thu, Feb 25, 2016 at 9:24 PM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, Feb 25, 2016 at 08:49:00AM +0530, Amitoj Kaur Chawla wrote:
>> With concurrency managed workqueues, use of dedicated workqueues
>> can be replaced by using system_wq. Drop usb_tx_wq and usb_rx_wq by
>> using system_wq.
>>
>> Since there are multiple work items per udev but different udevs
>> do not need to be ordered, increase of concurrency level by switching
>> to system_wq should not break anything.
>>
>> cancel_work_sync() is used to ensure that work is not pending or
>> executing on any CPU.
>>
>> Lastly, since all devices are suspended, which shutdowns the work item
>> before the driver can be unregistered, it is guaranteed that no work
>> item is pending or executing by the time exit path runs.
>>
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>
> Excellent work.  It'd be great if the somebody can test the changes on
> the actual hardware but for the workqueue part.
>
> Acked-by: Tejun Heo <tj@kernel.org>
>
> Thank a lot!
>
> --
> tejun

Thanks Tejun :)

Not sure how to go about testing on hardware. I guess the maintainers
should be cced?

Amitoj

Re: [Outreachy kernel] [PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[Tejun Heo]

Hello,

On Thu, Feb 25, 2016 at 11:40:17PM +0530, Amitoj Kaur Chawla wrote:
> > Excellent work.  It'd be great if the somebody can test the changes on
> > the actual hardware but for the workqueue part.
> >
> > Acked-by: Tejun Heo <tj@kernel.org>
> >
> > Thank a lot!
> >
> > --
> > tejun
> 
> Thanks Tejun :)
> 
> Not sure how to go about testing on hardware. I guess the maintainers
> should be cced?

Yeah, reposting the patch w/ maintainers cc'd should work, I suppose.

Thanks.

-- 
tejun

Re: [Outreachy kernel] [PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[Amitoj Kaur Chawla]

On Fri, Feb 26, 2016 at 2:51 AM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Thu, Feb 25, 2016 at 11:40:17PM +0530, Amitoj Kaur Chawla wrote:
>> > Excellent work.  It'd be great if the somebody can test the changes on
>> > the actual hardware but for the workqueue part.
>> >
>> > Acked-by: Tejun Heo <tj@kernel.org>
>> >
>> > Thank a lot!
>> >
>> > --
>> > tejun
>>
>> Thanks Tejun :)
>>
>> Not sure how to go about testing on hardware. I guess the maintainers
>> should be cced?
>
> Yeah, reposting the patch w/ maintainers cc'd should work, I suppose.
>
> Thanks.
>
> --
> tejun


I checked in getmaintainers.pl, seems like Greg is the maintainer for
both gdm724x and rtl8192e drivers. Should I be reposting in that case?

Amitoj

Re: [Outreachy kernel] [PATCH] staging: gdm724x: gdm_usb: Remove create_workqueue()

[Tejun Heo]

On Fri, Feb 26, 2016 at 12:57:49PM +0530, Amitoj Kaur Chawla wrote:
> I checked in getmaintainers.pl, seems like Greg is the maintainer for
> both gdm724x and rtl8192e drivers. Should I be reposting in that case?

Greg may or may not have actual hardware.  While getmaintainers can be
useful, actually looking at who did what can be a lot more useful.
For example, for gdm724x, run "git whatchanged -p
drivers/staging/gdm724x" and scan through the history.  Try to find
significant changes which aren't part of global cleanup or usage
update and see if there are people who are actively involved in those
changes as author, reviewer or committer.

For gdm724x, most of recent changes are mechanical, the original
author Won Kang's last change is from 2013, and Alexey Khoroshilov
seems to have done non-trival fixes recently, so I'd cc Won, Alexey
and Greg.

Thanks.

-- 
tejun