(no subject)

[castet.matthieu]

Hi,

could you CC me on ueagle-atm.c patches.

>From what I remind we sleep in the workqueue, that's why we couldn't use the
system one (freeze keyboard...). But may be the code changed.


Matthieu

Tejun Heo a écrit :
> With cmwq, there's no reason to use separate workqueues.  Drop
> uea_softc->work_q and use system_wq instead.  The used work item is
> sync flushed on driver detach.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Stanislaw Gruszka <stf_xl@wp.pl>
> Cc: linux-usb@vger.kernel.org
> ---
> Only compile tested.  Please feel free to take it into the subsystem
> tree or simply ack - I'll route it through the wq tree.
>
> Thanks.
>
>  drivers/usb/atm/ueagle-atm.c |   19 +++++--------------
>  1 files changed, 5 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/usb/atm/ueagle-atm.c b/drivers/usb/atm/ueagle-atm.c
> index 44447f5..55c1d3b 100644
> --- a/drivers/usb/atm/ueagle-atm.c
> +++ b/drivers/usb/atm/ueagle-atm.c
> @@ -168,7 +168,6 @@ struct uea_softc {
>  	union cmv_dsc cmv_dsc;
>
>  	struct work_struct task;
> -	struct workqueue_struct *work_q;
>  	u16 pageno;
>  	u16 ovl;
>
> @@ -1879,7 +1878,7 @@ static int uea_start_reset(struct uea_softc *sc)
>  	/* start loading DSP */
>  	sc->pageno = 0;
>  	sc->ovl = 0;
> -	queue_work(sc->work_q, &sc->task);
> +	schedule_work(&sc->task);
>
>  	/* wait for modem ready CMV */
>  	ret = wait_cmv_ack(sc);
> @@ -2091,14 +2090,14 @@ static void uea_schedule_load_page_e1(struct uea_softc
*sc,
>  {
>  	sc->pageno = intr->e1_bSwapPageNo;
>  	sc->ovl = intr->e1_bOvl >> 4 | intr->e1_bOvl << 4;
> -	queue_work(sc->work_q, &sc->task);
> +	schedule_work(&sc->task);
>  }
>
>  static void uea_schedule_load_page_e4(struct uea_softc *sc,
>  						struct intr_pkt *intr)
>  {
>  	sc->pageno = intr->e4_bSwapPageNo;
> -	queue_work(sc->work_q, &sc->task);
> +	schedule_work(&sc->task);
>  }
>
>  /*
> @@ -2170,13 +2169,6 @@ static int uea_boot(struct uea_softc *sc)
>
>  	init_waitqueue_head(&sc->sync_q);
>
> -	sc->work_q = create_workqueue("ueagle-dsp");
> -	if (!sc->work_q) {
> -		uea_err(INS_TO_USBDEV(sc), "cannot allocate workqueue\n");
> -		uea_leaves(INS_TO_USBDEV(sc));
> -		return -ENOMEM;
> -	}
> -
>  	if (UEA_CHIP_VERSION(sc) == ADI930)
>  		load_XILINX_firmware(sc);
>
> @@ -2222,7 +2214,6 @@ err1:
>  	sc->urb_int = NULL;
>  	kfree(intr);
>  err0:
> -	destroy_workqueue(sc->work_q);
>  	uea_leaves(INS_TO_USBDEV(sc));
>  	return -ENOMEM;
>  }
> @@ -2243,8 +2234,8 @@ static void uea_stop(struct uea_softc *sc)
>  	kfree(sc->urb_int->transfer_buffer);
>  	usb_free_urb(sc->urb_int);
>
> -	/* stop any pending boot process, when no one can schedule work */
> -	destroy_workqueue(sc->work_q);
> +	/* flush the work item, when no one can schedule it */
> +	flush_work_sync(&sc->task);
>
>  	if (sc->dsp_firm)
>  		release_firmware(sc->dsp_firm);

Re: [PATCH 19/32] usb/ueagle-atm: use system_wq instead of dedicated workqueues

[castet.matthieu]

Quoting castet.matthieu@free.fr:

> Hi,
>
> could you CC me on ueagle-atm.c patches.
>
> From what I remind we sleep in the workqueue, that's why we couldn't use the
> system one (freeze keyboard...). But may be the code changed.
For the record we still do blocking usb transfert on the workqueue.

>
>
> Matthieu
>
> Tejun Heo a écrit :
> > With cmwq, there's no reason to use separate workqueues.  Drop
> > uea_softc->work_q and use system_wq instead.  The used work item is
> > sync flushed on driver detach.
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Stanislaw Gruszka <stf_xl@wp.pl>
> > Cc: linux-usb@vger.kernel.org
> > ---
> > Only compile tested.  Please feel free to take it into the subsystem
> > tree or simply ack - I'll route it through the wq tree.
> >
> > Thanks.
> >
> >  drivers/usb/atm/ueagle-atm.c |   19 +++++--------------
> >  1 files changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/usb/atm/ueagle-atm.c b/drivers/usb/atm/ueagle-atm.c
> > index 44447f5..55c1d3b 100644
> > --- a/drivers/usb/atm/ueagle-atm.c
> > +++ b/drivers/usb/atm/ueagle-atm.c
> > @@ -168,7 +168,6 @@ struct uea_softc {
> >  	union cmv_dsc cmv_dsc;
> >
> >  	struct work_struct task;
> > -	struct workqueue_struct *work_q;
> >  	u16 pageno;
> >  	u16 ovl;
> >
> > @@ -1879,7 +1878,7 @@ static int uea_start_reset(struct uea_softc *sc)
> >  	/* start loading DSP */
> >  	sc->pageno = 0;
> >  	sc->ovl = 0;
> > -	queue_work(sc->work_q, &sc->task);
> > +	schedule_work(&sc->task);
> >
> >  	/* wait for modem ready CMV */
> >  	ret = wait_cmv_ack(sc);
> > @@ -2091,14 +2090,14 @@ static void uea_schedule_load_page_e1(struct
> uea_softc
> *sc,
> >  {
> >  	sc->pageno = intr->e1_bSwapPageNo;
> >  	sc->ovl = intr->e1_bOvl >> 4 | intr->e1_bOvl << 4;
> > -	queue_work(sc->work_q, &sc->task);
> > +	schedule_work(&sc->task);
> >  }
> >
> >  static void uea_schedule_load_page_e4(struct uea_softc *sc,
> >  						struct intr_pkt *intr)
> >  {
> >  	sc->pageno = intr->e4_bSwapPageNo;
> > -	queue_work(sc->work_q, &sc->task);
> > +	schedule_work(&sc->task);
> >  }
> >
> >  /*
> > @@ -2170,13 +2169,6 @@ static int uea_boot(struct uea_softc *sc)
> >
> >  	init_waitqueue_head(&sc->sync_q);
> >
> > -	sc->work_q = create_workqueue("ueagle-dsp");
> > -	if (!sc->work_q) {
> > -		uea_err(INS_TO_USBDEV(sc), "cannot allocate workqueue\n");
> > -		uea_leaves(INS_TO_USBDEV(sc));
> > -		return -ENOMEM;
> > -	}
> > -
> >  	if (UEA_CHIP_VERSION(sc) == ADI930)
> >  		load_XILINX_firmware(sc);
> >
> > @@ -2222,7 +2214,6 @@ err1:
> >  	sc->urb_int = NULL;
> >  	kfree(intr);
> >  err0:
> > -	destroy_workqueue(sc->work_q);
> >  	uea_leaves(INS_TO_USBDEV(sc));
> >  	return -ENOMEM;
> >  }
> > @@ -2243,8 +2234,8 @@ static void uea_stop(struct uea_softc *sc)
> >  	kfree(sc->urb_int->transfer_buffer);
> >  	usb_free_urb(sc->urb_int);
> >
> > -	/* stop any pending boot process, when no one can schedule work */
> > -	destroy_workqueue(sc->work_q);
> > +	/* flush the work item, when no one can schedule it */
> > +	flush_work_sync(&sc->task);
> >
> >  	if (sc->dsp_firm)
> >  		release_firmware(sc->dsp_firm);
>
>

Re: your mail

[Stanislaw Gruszka]

On Mon, Jan 03, 2011 at 05:38:00PM +0100, castet.matthieu@free.fr wrote:
> Hi,
> 
> could you CC me on ueagle-atm.c patches.
> 
> From what I remind we sleep in the workqueue, that's why we couldn't use the
> system one (freeze keyboard...). But may be the code changed.
In case when firmware is not available we can sleep for a few seconds in
work function. That's block keyboard driver who also use common workqueue.
If recent Tejun workqueue rewrite allow to long sleep in work func and
not hurt other workqueue users, patch is ok.

Unfortunately I'm not able to test the patch, my ueagle device was physically
damaged a few months ago.

Stanislaw

Re: your mail

[Tejun Heo]

On Mon, Jan 03, 2011 at 06:03:17PM +0100, Stanislaw Gruszka wrote:
> On Mon, Jan 03, 2011 at 05:38:00PM +0100, castet.matthieu@free.fr wrote:
> > could you CC me on ueagle-atm.c patches.

Will try to, but maybe it's a good idea to add a MAINTAINERS entry?

> > From what I remind we sleep in the workqueue, that's why we couldn't use the
> > system one (freeze keyboard...). But may be the code changed.
> In case when firmware is not available we can sleep for a few seconds in
> work function. That's block keyboard driver who also use common workqueue.
> If recent Tejun workqueue rewrite allow to long sleep in work func and
> not hurt other workqueue users, patch is ok.

Yeap, work items can sleep all they want on the system_wq.  It won't
delay execution of other work items.

Thanks.

--
tejun

[PATCH 19/32] usb/ueagle-atm: use system_wq instead of dedicated workqueues

[Tejun Heo]

With cmwq, there's no reason to use separate workqueues.  Drop
uea_softc->work_q and use system_wq instead.  The used work item is
sync flushed on driver detach.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Stanislaw Gruszka <stf_xl@wp.pl>
Cc: linux-usb@vger.kernel.org
---
Only compile tested.  Please feel free to take it into the subsystem
tree or simply ack - I'll route it through the wq tree.

Thanks.

 drivers/usb/atm/ueagle-atm.c |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/atm/ueagle-atm.c b/drivers/usb/atm/ueagle-atm.c
index 44447f5..55c1d3b 100644
--- a/drivers/usb/atm/ueagle-atm.c
+++ b/drivers/usb/atm/ueagle-atm.c
@@ -168,7 +168,6 @@ struct uea_softc {
 	union cmv_dsc cmv_dsc;
 
 	struct work_struct task;
-	struct workqueue_struct *work_q;
 	u16 pageno;
 	u16 ovl;
 
@@ -1879,7 +1878,7 @@ static int uea_start_reset(struct uea_softc *sc)
 	/* start loading DSP */
 	sc->pageno = 0;
 	sc->ovl = 0;
-	queue_work(sc->work_q, &sc->task);
+	schedule_work(&sc->task);
 
 	/* wait for modem ready CMV */
 	ret = wait_cmv_ack(sc);
@@ -2091,14 +2090,14 @@ static void uea_schedule_load_page_e1(struct uea_softc *sc,
 {
 	sc->pageno = intr->e1_bSwapPageNo;
 	sc->ovl = intr->e1_bOvl >> 4 | intr->e1_bOvl << 4;
-	queue_work(sc->work_q, &sc->task);
+	schedule_work(&sc->task);
 }
 
 static void uea_schedule_load_page_e4(struct uea_softc *sc,
 						struct intr_pkt *intr)
 {
 	sc->pageno = intr->e4_bSwapPageNo;
-	queue_work(sc->work_q, &sc->task);
+	schedule_work(&sc->task);
 }
 
 /*
@@ -2170,13 +2169,6 @@ static int uea_boot(struct uea_softc *sc)
 
 	init_waitqueue_head(&sc->sync_q);
 
-	sc->work_q = create_workqueue("ueagle-dsp");
-	if (!sc->work_q) {
-		uea_err(INS_TO_USBDEV(sc), "cannot allocate workqueue\n");
-		uea_leaves(INS_TO_USBDEV(sc));
-		return -ENOMEM;
-	}
-
 	if (UEA_CHIP_VERSION(sc) == ADI930)
 		load_XILINX_firmware(sc);
 
@@ -2222,7 +2214,6 @@ err1:
 	sc->urb_int = NULL;
 	kfree(intr);
 err0:
-	destroy_workqueue(sc->work_q);
 	uea_leaves(INS_TO_USBDEV(sc));
 	return -ENOMEM;
 }
@@ -2243,8 +2234,8 @@ static void uea_stop(struct uea_softc *sc)
 	kfree(sc->urb_int->transfer_buffer);
 	usb_free_urb(sc->urb_int);
 
-	/* stop any pending boot process, when no one can schedule work */
-	destroy_workqueue(sc->work_q);
+	/* flush the work item, when no one can schedule it */
+	flush_work_sync(&sc->task);
 
 	if (sc->dsp_firm)
 		release_firmware(sc->dsp_firm);
-- 
1.7.1