Re: [PATCH] usb/gadget: f_midi: Replace tasklet with work

[Takashi Iwai]

On Mon, 11 Jan 2021 05:28:55 +0100,
Davidlohr Bueso wrote:
> 
> Currently a tasklet is used to transmit input substream buffer
> data. However, tasklets have long been deprecated as being too
> heavy on the system by running in irq context - and this is not
> a performance critical path. If a higher priority process wants
> to run, it must wait for the tasklet to finish before doing so.
> 
> Deferring work to a workqueue and executing in process context
> should be fine considering the callback already does
> f_midi_do_transmit() under the transmit_lock and thus changes in
> semantics are ok regarding concurrency - tasklets being serialized
> against itself.
> 
> Cc: Takashi Iwai 
> Signed-off-by: Davidlohr Bueso 

The patch looks good to me.
Reviewed-by: Takashi Iwai 


BTW, now I found that the driver doesn't have the cancel call for
tasklet or work at the unbind.  Practically seen it's no big problem,
but its lack is a bait for syzbot.  We may need to just call
cancel_work_sync() somewhere, probably at f_midi_drop_out_substreams()
or f_midi_disable(), but it can be in another patch.


thanks,

Takashi

Re: [PATCH] usb/gadget: f_midi: Replace tasklet with work

[Felipe Balbi]

Hi,

Davidlohr Bueso  writes:
> Currently a tasklet is used to transmit input substream buffer
> data. However, tasklets have long been deprecated as being too
> heavy on the system by running in irq context - and this is not
> a performance critical path. If a higher priority process wants
> to run, it must wait for the tasklet to finish before doing so.
>
> Deferring work to a workqueue and executing in process context
> should be fine considering the callback already does
> f_midi_do_transmit() under the transmit_lock and thus changes in
> semantics are ok regarding concurrency - tasklets being serialized
> against itself.
>
> Cc: Takashi Iwai 
> Signed-off-by: Davidlohr Bueso 

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb/gadget: f_midi: Replace tasklet with work

[Davidlohr Bueso]

Currently a tasklet is used to transmit input substream buffer
data. However, tasklets have long been deprecated as being too
heavy on the system by running in irq context - and this is not
a performance critical path. If a higher priority process wants
to run, it must wait for the tasklet to finish before doing so.

Deferring work to a workqueue and executing in process context
should be fine considering the callback already does
f_midi_do_transmit() under the transmit_lock and thus changes in
semantics are ok regarding concurrency - tasklets being serialized
against itself.

Cc: Takashi Iwai 
Signed-off-by: Davidlohr Bueso 
---
 drivers/usb/gadget/function/f_midi.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index 8fff995b8dd5..71a1a26e85c7 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -87,7 +87,7 @@ struct f_midi {
struct snd_rawmidi_substream *out_substream[MAX_PORTS];
 
unsigned long   out_triggered;
-   struct tasklet_struct   tasklet;
+   struct work_struct  work;
unsigned int in_ports;
unsigned int out_ports;
int index;
@@ -698,9 +698,11 @@ static void f_midi_transmit(struct f_midi *midi)
f_midi_drop_out_substreams(midi);
 }
 
-static void f_midi_in_tasklet(struct tasklet_struct *t)
+static void f_midi_in_work(struct work_struct *work)
 {
-   struct f_midi *midi = from_tasklet(midi, t, tasklet);
+   struct f_midi *midi;
+
+   midi = container_of(work, struct f_midi, work);
f_midi_transmit(midi);
 }
 
@@ -737,7 +739,7 @@ static void f_midi_in_trigger(struct snd_rawmidi_substream 
*substream, int up)
VDBG(midi, "%s() %d\n", __func__, up);
midi->in_ports_array[substream->number].active = up;
if (up)
-   tasklet_hi_schedule(>tasklet);
+   queue_work(system_highpri_wq, >work);
 }
 
 static int f_midi_out_open(struct snd_rawmidi_substream *substream)
@@ -875,7 +877,7 @@ static int f_midi_bind(struct usb_configuration *c, struct 
usb_function *f)
int status, n, jack = 1, i = 0, endpoint_descriptor_index = 0;
 
midi->gadget = cdev->gadget;
-   tasklet_setup(>tasklet, f_midi_in_tasklet);
+   INIT_WORK(>work, f_midi_in_work);
status = f_midi_register_card(midi);
if (status < 0)
goto fail_register;
-- 
2.26.2