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 <[email protected]>
Signed-off-by: Davidlohr Bueso <[email protected]>
---
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(&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;
--
2.26.2
Hi,
Davidlohr Bueso <[email protected]> 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 <[email protected]>
> Signed-off-by: Davidlohr Bueso <[email protected]>
Acked-by: Felipe Balbi <[email protected]>
--
balbi
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 <[email protected]>
> Signed-off-by: Davidlohr Bueso <[email protected]>
The patch looks good to me.
Reviewed-by: Takashi Iwai <[email protected]>
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