[alsa-devel] [PATCH 0/4] ALSA: line6: Cleanup startup sequences
Hi,
while debugging line6 toneport driver bug, I noticed that other line6 drivers have too complex and fragile startup sequences. This is the result of the rewrites with a single delayed work. Maybe for 5.3, as these are no critical fixes.
Takashi
===
Takashi Iwai (4): ALSA: line6: pod: Rewrite complex timer & work combo with a delayed work ALSA: line6: podhd: Rewrite complex timer & work combo with a delayed work ALSA: line6: variax: Rewrite complex timer & work combo with a delayed work ALSA: line6: Drop superfluous timer helper function
sound/usb/line6/driver.c | 11 ----- sound/usb/line6/driver.h | 9 ---- sound/usb/line6/pod.c | 85 ++++++++++++-------------------- sound/usb/line6/podhd.c | 73 +++++---------------------- sound/usb/line6/variax.c | 125 ++++++++++++++--------------------------------- 5 files changed, 78 insertions(+), 225 deletions(-)
The POD driver had a complex staged startup procedure using both timer and work. This patch simplifies it via a single delayed work with the reduced stages.
Now basically only two intermediate stages: - POD_STARTUP_VERSIONREQ: requesting the version information and the process_message callback triggers the next stage, - POD_STARTUP_SETUP: registering the actual card object.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/pod.c | 85 ++++++++++++++++++--------------------------------- 1 file changed, 30 insertions(+), 55 deletions(-)
diff --git a/sound/usb/line6/pod.c b/sound/usb/line6/pod.c index ce45b6dab651..95d4b35ca0af 100644 --- a/sound/usb/line6/pod.c +++ b/sound/usb/line6/pod.c @@ -39,11 +39,9 @@ Stages of POD startup procedure */ enum { - POD_STARTUP_INIT = 1, POD_STARTUP_VERSIONREQ, - POD_STARTUP_WORKQUEUE, POD_STARTUP_SETUP, - POD_STARTUP_LAST = POD_STARTUP_SETUP - 1 + POD_STARTUP_DONE, };
enum { @@ -63,11 +61,8 @@ struct usb_line6_pod { /* Instrument monitor level */ int monitor_level;
- /* Timer for device initialization */ - struct timer_list startup_timer; - /* Work handler for device initialization */ - struct work_struct startup_work; + struct delayed_work startup_work;
/* Current progress in startup procedure */ int startup_progress; @@ -173,10 +168,6 @@ static const char pod_version_header[] = { 0xf2, 0x7e, 0x7f, 0x06, 0x02 };
-/* forward declarations: */ -static void pod_startup2(struct timer_list *t); -static void pod_startup3(struct usb_line6_pod *pod); - static char *pod_alloc_sysex_buffer(struct usb_line6_pod *pod, int code, int size) { @@ -196,7 +187,10 @@ static void line6_pod_process_message(struct usb_line6 *line6) pod->firmware_version = buf[13] * 100 + buf[14] * 10 + buf[15]; pod->device_id = ((int)buf[8] << 16) | ((int)buf[9] << 8) | (int) buf[10]; - pod_startup3(pod); + if (pod->startup_progress == POD_STARTUP_VERSIONREQ) { + pod->startup_progress = POD_STARTUP_SETUP; + schedule_delayed_work(&pod->startup_work, 0); + } return; }
@@ -281,47 +275,29 @@ static ssize_t device_id_show(struct device *dev, context). After the last one has finished, the device is ready to use. */
-static void pod_startup1(struct usb_line6_pod *pod) -{ - CHECK_STARTUP_PROGRESS(pod->startup_progress, POD_STARTUP_INIT); - - /* delay startup procedure: */ - line6_start_timer(&pod->startup_timer, POD_STARTUP_DELAY, pod_startup2); -} - -static void pod_startup2(struct timer_list *t) +static void pod_startup_work(struct work_struct *work) { - struct usb_line6_pod *pod = from_timer(pod, t, startup_timer); + struct usb_line6_pod *pod = container_of(work, struct usb_line6_pod, + startup_work.work); struct usb_line6 *line6 = &pod->line6;
- CHECK_STARTUP_PROGRESS(pod->startup_progress, POD_STARTUP_VERSIONREQ); - - /* request firmware version: */ - line6_version_request_async(line6); -} - -static void pod_startup3(struct usb_line6_pod *pod) -{ - CHECK_STARTUP_PROGRESS(pod->startup_progress, POD_STARTUP_WORKQUEUE); - - /* schedule work for global work queue: */ - schedule_work(&pod->startup_work); -} - -static void pod_startup4(struct work_struct *work) -{ - struct usb_line6_pod *pod = - container_of(work, struct usb_line6_pod, startup_work); - struct usb_line6 *line6 = &pod->line6; - - CHECK_STARTUP_PROGRESS(pod->startup_progress, POD_STARTUP_SETUP); - - /* serial number: */ - line6_read_serial_number(&pod->line6, &pod->serial_number); - - /* ALSA audio interface: */ - if (snd_card_register(line6->card)) - dev_err(line6->ifcdev, "Failed to register POD card.\n"); + switch (pod->startup_progress) { + case POD_STARTUP_VERSIONREQ: + /* request firmware version: */ + line6_version_request_async(line6); + break; + case POD_STARTUP_SETUP: + /* serial number: */ + line6_read_serial_number(&pod->line6, &pod->serial_number); + + /* ALSA audio interface: */ + if (snd_card_register(line6->card)) + dev_err(line6->ifcdev, "Failed to register POD card.\n"); + pod->startup_progress = POD_STARTUP_DONE; + break; + default: + break; + } }
/* POD special files: */ @@ -397,8 +373,7 @@ static void line6_pod_disconnect(struct usb_line6 *line6) { struct usb_line6_pod *pod = (struct usb_line6_pod *)line6;
- del_timer_sync(&pod->startup_timer); - cancel_work_sync(&pod->startup_work); + cancel_delayed_work_sync(&pod->startup_work); }
/* @@ -413,8 +388,7 @@ static int pod_init(struct usb_line6 *line6, line6->process_message = line6_pod_process_message; line6->disconnect = line6_pod_disconnect;
- timer_setup(&pod->startup_timer, NULL, 0); - INIT_WORK(&pod->startup_work, pod_startup4); + INIT_DELAYED_WORK(&pod->startup_work, pod_startup_work);
/* create sysfs entries: */ err = snd_card_add_dev_attr(line6->card, &pod_dev_attr_group); @@ -447,7 +421,8 @@ static int pod_init(struct usb_line6 *line6, pod->monitor_level = POD_SYSTEM_INVALID;
/* initiate startup procedure: */ - pod_startup1(pod); + schedule_delayed_work(&pod->startup_work, + msecs_to_jiffies(POD_STARTUP_DELAY)); }
return 0;
POD HD driver had a complex staged startup sequence with both timer and work. This patch simplifies it to a single delayed work with a single stage.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/podhd.c | 73 ++++++++----------------------------------------- 1 file changed, 11 insertions(+), 62 deletions(-)
diff --git a/sound/usb/line6/podhd.c b/sound/usb/line6/podhd.c index 5f3c87264e66..be5d15639574 100644 --- a/sound/usb/line6/podhd.c +++ b/sound/usb/line6/podhd.c @@ -22,16 +22,6 @@
#define PODHD_STARTUP_DELAY 500
-/* - * Stages of POD startup procedure - */ -enum { - PODHD_STARTUP_INIT = 1, - PODHD_STARTUP_SCHEDULE_WORKQUEUE, - PODHD_STARTUP_SETUP, - PODHD_STARTUP_LAST = PODHD_STARTUP_SETUP - 1 -}; - enum { LINE6_PODHD300, LINE6_PODHD400, @@ -47,14 +37,8 @@ struct usb_line6_podhd { /* Generic Line 6 USB data */ struct usb_line6 line6;
- /* Timer for device initialization */ - struct timer_list startup_timer; - /* Work handler for device initialization */ - struct work_struct startup_work; - - /* Current progress in startup procedure */ - int startup_progress; + struct delayed_work startup_work;
/* Serial number of device */ u32 serial_number; @@ -158,10 +142,6 @@ static struct line6_pcm_properties podx3_pcm_properties = { }; static struct usb_driver podhd_driver;
-static void podhd_startup_start_workqueue(struct timer_list *t); -static void podhd_startup_workqueue(struct work_struct *work); -static int podhd_startup_finalize(struct usb_line6_podhd *pod); - static ssize_t serial_number_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -202,26 +182,6 @@ static const struct attribute_group podhd_dev_attr_group = { * audio nor bulk interfaces to work. */
-static void podhd_startup(struct usb_line6_podhd *pod) -{ - CHECK_STARTUP_PROGRESS(pod->startup_progress, PODHD_STARTUP_INIT); - - /* delay startup procedure: */ - line6_start_timer(&pod->startup_timer, PODHD_STARTUP_DELAY, - podhd_startup_start_workqueue); -} - -static void podhd_startup_start_workqueue(struct timer_list *t) -{ - struct usb_line6_podhd *pod = from_timer(pod, t, startup_timer); - - CHECK_STARTUP_PROGRESS(pod->startup_progress, - PODHD_STARTUP_SCHEDULE_WORKQUEUE); - - /* schedule work for global work queue: */ - schedule_work(&pod->startup_work); -} - static int podhd_dev_start(struct usb_line6_podhd *pod) { int ret; @@ -272,37 +232,27 @@ static int podhd_dev_start(struct usb_line6_podhd *pod) return ret; }
-static void podhd_startup_workqueue(struct work_struct *work) +static void podhd_startup_work(struct work_struct *work) { struct usb_line6_podhd *pod = - container_of(work, struct usb_line6_podhd, startup_work); - - CHECK_STARTUP_PROGRESS(pod->startup_progress, PODHD_STARTUP_SETUP); + container_of(work, struct usb_line6_podhd, startup_work.work); + struct usb_line6 *line6 = &pod->line6;
podhd_dev_start(pod); line6_read_serial_number(&pod->line6, &pod->serial_number); - - podhd_startup_finalize(pod); -} - -static int podhd_startup_finalize(struct usb_line6_podhd *pod) -{ - struct usb_line6 *line6 = &pod->line6; - - /* ALSA audio interface: */ - return snd_card_register(line6->card); + if (snd_card_register(line6->card)) + dev_err(line6->ifcdev, "Failed to register POD HD card.\n"); }
static void podhd_disconnect(struct usb_line6 *line6) { struct usb_line6_podhd *pod = (struct usb_line6_podhd *)line6;
+ cancel_delayed_work_sync(&pod->startup_work); + if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL_INFO) { struct usb_interface *intf;
- del_timer_sync(&pod->startup_timer); - cancel_work_sync(&pod->startup_work); - intf = usb_ifnum_to_if(line6->usbdev, pod->line6.properties->ctrl_if); if (intf) @@ -322,8 +272,7 @@ static int podhd_init(struct usb_line6 *line6,
line6->disconnect = podhd_disconnect;
- timer_setup(&pod->startup_timer, NULL, 0); - INIT_WORK(&pod->startup_work, podhd_startup_workqueue); + INIT_DELAYED_WORK(&pod->startup_work, podhd_startup_work);
if (pod->line6.properties->capabilities & LINE6_CAP_CONTROL) { /* claim the data interface */ @@ -362,11 +311,11 @@ static int podhd_init(struct usb_line6 *line6,
if (!(pod->line6.properties->capabilities & LINE6_CAP_CONTROL_INFO)) { /* register USB audio system directly */ - return podhd_startup_finalize(pod); + return snd_card_register(line6->card); }
/* init device and delay registering */ - podhd_startup(pod); + schedule_delayed_work(&pod->startup_work, PODHD_STARTUP_DELAY); return 0; }
Variax driver had a very complex and staged startup sequence using multiple timers and a work. This patch simplifies the procedure to a single delayed work.
Now the startup stage consists of: - VARIAX_STARTUP_VERSIONREQ: requesting the version and the message handler raises up to the next stage upon receiving the reply. The request is repeated until a reply arrives. - VARIAX_STARTUP_ACTIVATE: does activation, and queue for the next stage. - VARIAX_STARTUP_SETUP: registers the card.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/variax.c | 125 ++++++++++++++--------------------------------- 1 file changed, 37 insertions(+), 88 deletions(-)
diff --git a/sound/usb/line6/variax.c b/sound/usb/line6/variax.c index e8c852b2ce35..b56a993dd415 100644 --- a/sound/usb/line6/variax.c +++ b/sound/usb/line6/variax.c @@ -26,13 +26,9 @@ Stages of Variax startup procedure */ enum { - VARIAX_STARTUP_INIT = 1, VARIAX_STARTUP_VERSIONREQ, - VARIAX_STARTUP_WAIT, VARIAX_STARTUP_ACTIVATE, - VARIAX_STARTUP_WORKQUEUE, VARIAX_STARTUP_SETUP, - VARIAX_STARTUP_LAST = VARIAX_STARTUP_SETUP - 1 };
enum { @@ -48,11 +44,7 @@ struct usb_line6_variax { unsigned char *buffer_activate;
/* Handler for device initialization */ - struct work_struct startup_work; - - /* Timers for device initialization */ - struct timer_list startup_timer1; - struct timer_list startup_timer2; + struct delayed_work startup_work;
/* Current progress in startup procedure */ int startup_progress; @@ -81,11 +73,6 @@ static const char variax_activate[] = { 0xf7 };
-/* forward declarations: */ -static void variax_startup2(struct timer_list *t); -static void variax_startup4(struct timer_list *t); -static void variax_startup5(struct timer_list *t); - static void variax_activate_async(struct usb_line6_variax *variax, int a) { variax->buffer_activate[VARIAX_OFFSET_ACTIVATE] = a; @@ -100,74 +87,32 @@ static void variax_activate_async(struct usb_line6_variax *variax, int a) context). After the last one has finished, the device is ready to use. */
-static void variax_startup1(struct usb_line6_variax *variax) -{ - CHECK_STARTUP_PROGRESS(variax->startup_progress, VARIAX_STARTUP_INIT); - - /* delay startup procedure: */ - line6_start_timer(&variax->startup_timer1, VARIAX_STARTUP_DELAY1, - variax_startup2); -} - -static void variax_startup2(struct timer_list *t) -{ - struct usb_line6_variax *variax = from_timer(variax, t, startup_timer1); - struct usb_line6 *line6 = &variax->line6; - - /* schedule another startup procedure until startup is complete: */ - if (variax->startup_progress >= VARIAX_STARTUP_LAST) - return; - - variax->startup_progress = VARIAX_STARTUP_VERSIONREQ; - line6_start_timer(&variax->startup_timer1, VARIAX_STARTUP_DELAY1, - variax_startup2); - - /* request firmware version: */ - line6_version_request_async(line6); -} - -static void variax_startup3(struct usb_line6_variax *variax) -{ - CHECK_STARTUP_PROGRESS(variax->startup_progress, VARIAX_STARTUP_WAIT); - - /* delay startup procedure: */ - line6_start_timer(&variax->startup_timer2, VARIAX_STARTUP_DELAY3, - variax_startup4); -} - -static void variax_startup4(struct timer_list *t) -{ - struct usb_line6_variax *variax = from_timer(variax, t, startup_timer2); - - CHECK_STARTUP_PROGRESS(variax->startup_progress, - VARIAX_STARTUP_ACTIVATE); - - /* activate device: */ - variax_activate_async(variax, 1); - line6_start_timer(&variax->startup_timer2, VARIAX_STARTUP_DELAY4, - variax_startup5); -} - -static void variax_startup5(struct timer_list *t) -{ - struct usb_line6_variax *variax = from_timer(variax, t, startup_timer2); - - CHECK_STARTUP_PROGRESS(variax->startup_progress, - VARIAX_STARTUP_WORKQUEUE); - - /* schedule work for global work queue: */ - schedule_work(&variax->startup_work); -} - -static void variax_startup6(struct work_struct *work) +static void variax_startup_work(struct work_struct *work) { struct usb_line6_variax *variax = - container_of(work, struct usb_line6_variax, startup_work); - - CHECK_STARTUP_PROGRESS(variax->startup_progress, VARIAX_STARTUP_SETUP); + container_of(work, struct usb_line6_variax, startup_work.work); + struct usb_line6 *line6 = &variax->line6;
- /* ALSA audio interface: */ - snd_card_register(variax->line6.card); + switch (variax->startup_progress) { + case VARIAX_STARTUP_VERSIONREQ: + /* repeat request until getting the response */ + schedule_delayed_work(&variax->startup_work, + VARIAX_STARTUP_DELAY1); + /* request firmware version: */ + line6_version_request_async(line6); + break; + case VARIAX_STARTUP_ACTIVATE: + /* activate device: */ + variax_activate_async(variax, 1); + variax->startup_progress = VARIAX_STARTUP_SETUP; + schedule_delayed_work(&variax->startup_work, + VARIAX_STARTUP_DELAY4); + break; + case VARIAX_STARTUP_SETUP: + /* ALSA audio interface: */ + snd_card_register(variax->line6.card); + break; + } }
/* @@ -186,11 +131,19 @@ static void line6_variax_process_message(struct usb_line6 *line6) case LINE6_SYSEX_BEGIN: if (memcmp(buf + 1, variax_init_version + 1, sizeof(variax_init_version) - 1) == 0) { - variax_startup3(variax); + if (variax->startup_progress >= VARIAX_STARTUP_ACTIVATE) + break; + variax->startup_progress = VARIAX_STARTUP_ACTIVATE; + cancel_delayed_work(&variax->startup_work); + schedule_delayed_work(&variax->startup_work, + VARIAX_STARTUP_DELAY3); } else if (memcmp(buf + 1, variax_init_done + 1, sizeof(variax_init_done) - 1) == 0) { /* notify of complete initialization: */ - variax_startup4(&variax->startup_timer2); + if (variax->startup_progress >= VARIAX_STARTUP_SETUP) + break; + cancel_delayed_work(&variax->startup_work); + schedule_delayed_work(&variax->startup_work, 0); } break; } @@ -203,9 +156,7 @@ static void line6_variax_disconnect(struct usb_line6 *line6) { struct usb_line6_variax *variax = (struct usb_line6_variax *)line6;
- del_timer(&variax->startup_timer1); - del_timer(&variax->startup_timer2); - cancel_work_sync(&variax->startup_work); + cancel_delayed_work_sync(&variax->startup_work);
kfree(variax->buffer_activate); } @@ -222,9 +173,7 @@ static int variax_init(struct usb_line6 *line6, line6->process_message = line6_variax_process_message; line6->disconnect = line6_variax_disconnect;
- timer_setup(&variax->startup_timer1, NULL, 0); - timer_setup(&variax->startup_timer2, NULL, 0); - INIT_WORK(&variax->startup_work, variax_startup6); + INIT_DELAYED_WORK(&variax->startup_work, variax_startup_work);
/* initialize USB buffers: */ variax->buffer_activate = kmemdup(variax_activate, @@ -239,7 +188,7 @@ static int variax_init(struct usb_line6 *line6, return err;
/* initiate startup procedure: */ - variax_startup1(variax); + schedule_delayed_work(&variax->startup_work, VARIAX_STARTUP_DELAY1); return 0; }
Now all timer usages in line6 drivers are gone, we can get rid of some helper macro and function that became superfluous.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/line6/driver.c | 11 ----------- sound/usb/line6/driver.h | 9 --------- 2 files changed, 20 deletions(-)
diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index b61f65bed4e4..b1c9be4bbd0f 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -195,17 +195,6 @@ static int line6_send_raw_message_async_part(struct message *msg, return retval; }
-/* - Setup and start timer. -*/ -void line6_start_timer(struct timer_list *timer, unsigned long msecs, - void (*function)(struct timer_list *t)) -{ - timer->function = function; - mod_timer(timer, jiffies + msecs_to_jiffies(msecs)); -} -EXPORT_SYMBOL_GPL(line6_start_timer); - /* Asynchronously send raw message. */ diff --git a/sound/usb/line6/driver.h b/sound/usb/line6/driver.h index 61425597eb61..0bd6a5cff539 100644 --- a/sound/usb/line6/driver.h +++ b/sound/usb/line6/driver.h @@ -68,13 +68,6 @@
#define LINE6_CHANNEL_MASK 0x0f
-#define CHECK_STARTUP_PROGRESS(x, n) \ -do { \ - if ((x) >= (n)) \ - return; \ - x = (n); \ -} while (0) - extern const unsigned char line6_midi_id[3];
static const int SYSEX_DATA_OFS = sizeof(line6_midi_id) + 3; @@ -197,8 +190,6 @@ extern int line6_send_sysex_message(struct usb_line6 *line6, const char *buffer, int size); extern ssize_t line6_set_raw(struct device *dev, struct device_attribute *attr, const char *buf, size_t count); -extern void line6_start_timer(struct timer_list *timer, unsigned long msecs, - void (*function)(struct timer_list *t)); extern int line6_version_request_async(struct usb_line6 *line6); extern int line6_write_data(struct usb_line6 *line6, unsigned address, void *data, unsigned datalen);
participants (1)
-
Takashi Iwai