| Message ID | 20210111042855.73289-1-dave@stgolabs.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8653d71ce3763aedcf3d2331f59beda3fecd79e4 |
| Headers | show |
| Series | usb/gadget: f_midi: Replace tasklet with work | expand |
Hi, Davidlohr Bueso <dave@stgolabs.net> 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 <tiwai@suse.de> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de> Acked-by: Felipe Balbi <balbi@kernel.org>
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 <tiwai@suse.de> > Signed-off-by: Davidlohr Bueso <dbueso@suse.de> The patch looks good to me. Reviewed-by: Takashi Iwai <tiwai@suse.de> 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
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(&midi->tasklet); + queue_work(system_highpri_wq, &midi->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(&midi->tasklet, f_midi_in_tasklet); + INIT_WORK(&midi->work, f_midi_in_work); status = f_midi_register_card(midi); if (status < 0) goto fail_register;
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 <tiwai@suse.de> Signed-off-by: Davidlohr Bueso <dbueso@suse.de> --- drivers/usb/gadget/function/f_midi.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)