mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
To: barebox@lists.infradead.org
Cc: Ahmad Fatoum <a.fatoum@pengutronix.de>
Subject: [PATCH 2/3] bthread: replace blocking bthread_stop with nonblocking bthread_cancel
Date: Mon, 28 Jun 2021 09:07:31 +0200	[thread overview]
Message-ID: <20210628070732.16812-2-a.fatoum@pengutronix.de> (raw)
In-Reply-To: <20210628070732.16812-1-a.fatoum@pengutronix.de>

When bthread were first merged, they could be scheduled in any context
and bthread_stop could just keep rescheduling until the bthread in
question exits after which it would return the exit code.

Now that bthreads are only scheduled in command context, bthread_stop
also can only be scheduled in command context, making it much less
useful and easier to shoot yourself in the foot with.

Avoid this by introducing a bthread_cancel function instead that will
asynchronously terminate the thread. For most purposes that should be
fine, because bthread_stop is used to synchronize cleanup and we can
move the cleanup into the thread instead.

The only exception is the bthread command, which relies on being able to
wait on bthreads to complete. For these __bthread_stop remains available,
but should not be used in new code.

This fixes a hang that is encountered when the usb mass storage gadget
unbind is called from a poller leading barebox to wait indefinitely.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/bthread.c                  |  3 --
 common/bthread.c                    | 44 ++++++++++++++++++-------
 drivers/usb/gadget/f_mass_storage.c | 51 +++++++++++++----------------
 include/bthread.h                   |  2 +-
 4 files changed, 56 insertions(+), 44 deletions(-)

diff --git a/commands/bthread.c b/commands/bthread.c
index 446fe9c37ac3..ce3db2d3d292 100644
--- a/commands/bthread.c
+++ b/commands/bthread.c
@@ -61,7 +61,6 @@ static int bthread_isolated_time(void)
 	}
 
 	__bthread_stop(bthread);
-	bthread_free(bthread);
 
 	return i;
 }
@@ -120,7 +119,6 @@ cleanup:
 	while (i--) {
 		arg = bthread_data(bthread[i]);
 		__bthread_stop(bthread[i]);
-		bthread_free(bthread[i]);
 
 		if (!ret && (arg->out != 4 || yields < arg->out))
 			ret = -EIO;
@@ -184,7 +182,6 @@ cleanup:
 	list_for_each_entry_safe(spawner, tmp, &spawners, list) {
 		arg = bthread_data(spawner->bthread);
 		__bthread_stop(spawner->bthread);
-		bthread_free(spawner->bthread);
 		if (!ret && arg->out)
 			ret = arg->out;
 		free(arg);
diff --git a/common/bthread.c b/common/bthread.c
index 48248dfad41a..46e6987149bb 100644
--- a/common/bthread.c
+++ b/common/bthread.c
@@ -32,10 +32,12 @@ static struct bthread {
 #endif
 	u8 awake :1;
 	u8 should_stop :1;
+	u8 should_clean :1;
 	u8 has_stopped :1;
 } main_thread = {
 	.list = LIST_HEAD_INIT(main_thread.list),
 	.name = "main",
+	.awake = true,
 };
 
 struct bthread *current = &main_thread;
@@ -66,7 +68,7 @@ bool bthread_is_main(struct bthread *bthread)
 	return bthread == &main_thread;
 }
 
-void bthread_free(struct bthread *bthread)
+static void bthread_free(struct bthread *bthread)
 {
 	free(bthread->stack);
 	free(bthread->name);
@@ -104,6 +106,8 @@ struct bthread *bthread_create(void (*threadfn)(void *), void *data,
 	if (len < 0)
 		goto err;
 
+	list_add_tail(&bthread->list, &current->list);
+
 	/* set up bthread context with the new stack */
 	initjmp(bthread->jmp_buf, bthread_trampoline,
 		bthread->stack + CONFIG_STACK_SIZE);
@@ -121,26 +125,31 @@ void *bthread_data(struct bthread *bthread)
 
 void bthread_wake(struct bthread *bthread)
 {
-	if (bthread->awake)
-		return;
-	list_add_tail(&bthread->list, &current->list);
 	bthread->awake = true;
 }
 
 void bthread_suspend(struct bthread *bthread)
 {
-	if (!bthread->awake)
-		return;
 	bthread->awake = false;
-	list_del(&bthread->list);
+}
+
+void bthread_cancel(struct bthread *bthread)
+{
+	bthread->should_stop = true;
+	bthread->should_clean = true;
 }
 
 void __bthread_stop(struct bthread *bthread)
 {
 	bthread->should_stop = true;
 
+	pr_debug("waiting for %s to stop\n", bthread->name);
+
 	while (!bthread->has_stopped)
 		bthread_reschedule();
+
+	list_del(&bthread->list);
+	bthread_free(bthread);
 }
 
 int bthread_should_stop(void)
@@ -163,10 +172,23 @@ void bthread_info(void)
 
 void bthread_reschedule(void)
 {
-	struct bthread *next = list_next_entry(current, list);
-	if (current != next)
-		pr_debug("switch %s -> %s\n", current->name, next->name);
-	bthread_schedule(next);
+	struct bthread *next, *tmp;
+
+	if (current == list_next_entry(current, list))
+		return;
+
+	list_for_each_entry_safe(next, tmp, &current->list, list) {
+		if (next->awake) {
+			pr_debug("switch %s -> %s\n", current->name, next->name);
+			bthread_schedule(next);
+			return;
+		}
+		if (next->has_stopped && next->should_clean) {
+			pr_debug("deleting %s\n", next->name);
+			list_del(&next->list);
+			bthread_free(next);
+		}
+	}
 }
 
 void bthread_schedule(struct bthread *to)
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c
index 0033a95f68f3..753042125d14 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2335,9 +2335,12 @@ static void handle_exception(struct fsg_common *common)
 
 /*-------------------------------------------------------------------------*/
 
-static void fsg_main_thread(void *common_)
+static void fsg_main_thread(void *fsg_)
 {
-	struct fsg_common	*common = common_;
+	struct fsg_dev *fsg = fsg_;
+	struct fsg_common *common = fsg->common;
+	struct fsg_buffhd *bh;
+	unsigned i;
 	int ret = 0;
 
 	/* The main loop */
@@ -2376,6 +2379,21 @@ static void fsg_main_thread(void *common_)
 
 	if (ret && ret != -ERESTARTSYS)
 		pr_warn("%s: error %pe\n", __func__, ERR_PTR(ret));
+
+	usb_free_all_descriptors(&fsg->function);
+
+	for (i = 0; i < ums_count; i++)
+		close(ums[i].fd);
+
+	bh = common->buffhds;
+	i = FSG_NUM_BUFFERS;
+
+	do {
+		dma_free(bh->buf);
+	} while (++bh, --i);
+
+	ums_count = 0;
+	ums_files = NULL;
 }
 
 static void fsg_common_release(struct fsg_common *common);
@@ -2409,7 +2427,7 @@ static int fsg_common_init(struct fsg_common *common,
 	common->ep0 = gadget->ep0;
 	common->ep0req = cdev->req;
 
-	thread_task = bthread_run(fsg_main_thread, common, "mass-storage-gadget");
+	thread_task = bthread_run(fsg_main_thread, common->fsg, "mass-storage-gadget");
 	if (IS_ERR(thread_task))
 		return PTR_ERR(thread_task);
 
@@ -2531,52 +2549,27 @@ close:
 
 static void fsg_common_release(struct fsg_common *common)
 {
-	struct fsg_buffhd *bh;
-	unsigned i;
-
 	/* If the thread isn't already dead, tell it to exit now */
 	if (common->state != FSG_STATE_TERMINATED) {
 		raise_exception(common, FSG_STATE_EXIT);
-		__bthread_stop(thread_task);
-		bthread_free(thread_task);
 	}
 
-	bh = common->buffhds;
-	i = FSG_NUM_BUFFERS;
-
-	do {
-		dma_free(bh->buf);
-	} while (++bh, --i);
-
-	ums_count = 0;
+	bthread_cancel(thread_task);
 }
 
 
 static void fsg_unbind(struct usb_configuration *c, struct usb_function *f)
 {
 	struct fsg_dev		*fsg = fsg_from_func(f);
-	struct fsg_common	*common = fsg->common;
-	int i;
 
 	DBG(fsg, "unbind\n");
 
 	if (fsg->common->fsg == fsg) {
 		fsg->common->new_fsg = NULL;
 		raise_exception(fsg->common, FSG_STATE_CONFIG_CHANGE);
-
-		__bthread_stop(thread_task);
-		while (common->fsg == fsg)
-			bthread_reschedule();
 	}
 
-	usb_free_all_descriptors(&fsg->function);
-
-	for (i = 0; i < ums_count; i++)
-		close(ums[i].fd);
-
 	fsg_common_release(fsg->common);
-
-	ums_files = NULL;
 }
 
 static int fsg_bind(struct usb_configuration *c, struct usb_function *f)
diff --git a/include/bthread.h b/include/bthread.h
index 407aa830a835..4441b53696fb 100644
--- a/include/bthread.h
+++ b/include/bthread.h
@@ -13,7 +13,7 @@ struct bthread;
 extern struct bthread *current;
 
 struct bthread *bthread_create(void (*threadfn)(void *), void *data, const char *namefmt, ...);
-void bthread_free(struct bthread *bthread);
+void bthread_cancel(struct bthread *bthread);
 
 void bthread_schedule(struct bthread *);
 void bthread_wake(struct bthread *bthread);
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


  reply	other threads:[~2021-06-28  7:10 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-28  7:07 [PATCH 1/3] bthread: remove thread exit codes Ahmad Fatoum
2021-06-28  7:07 ` Ahmad Fatoum [this message]
2021-06-28  7:07 ` [PATCH 3/3] bthread: add options to create and delete dummy bthreads Ahmad Fatoum
2021-06-28 12:45 ` [PATCH 1/3] bthread: remove thread exit codes Sascha Hauer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210628070732.16812-2-a.fatoum@pengutronix.de \
    --to=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --subject='Re: [PATCH 2/3] bthread: replace blocking bthread_stop with nonblocking bthread_cancel' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox