[PATCH] usbgadget: autostart: add usbgadget_autostart helper for board code

[PATCH] usbgadget: autostart: add usbgadget_autostart helper for board code

[Ahmad Fatoum]

barebox will rerun a setter if a nv variable changes its global variable's
default. If setter is accessing other nv variables though, the setter
will not rerun when they change. For this reason, usbgadget_autostart_init()
must be registered at postenvironment_initcall() level, but this makes
the variable only reliably usable from the shell as there's no later
initcall level than postenvironment_initcall() for board code to run at.

Add a new usbgadget_autostart(boot enable) function that board code can
use instead.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/usbgadget.c         | 15 ++++++++++++++-
 include/usb/gadget-multi.h |  3 +++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/common/usbgadget.c b/common/usbgadget.c
index 2ec6d9226cca..161fd16fedb7 100644
--- a/common/usbgadget.c
+++ b/common/usbgadget.c
@@ -121,6 +121,12 @@ static int usbgadget_autostart_set(struct param_d *param, void *ctx)
 	return err;
 }
 
+void usbgadget_autostart(bool enable)
+{
+	globalvar_set("usbgadget.autostart", enable ? "1" : "0");
+	autostart = enable;
+}
+
 static int usbgadget_globalvars_init(void)
 {
 	globalvar_add_simple_bool("usbgadget.acm", &acm);
@@ -132,8 +138,15 @@ device_initcall(usbgadget_globalvars_init);
 
 static int usbgadget_autostart_init(void)
 {
-	if (IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART))
+	if (IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART)) {
 		globalvar_add_bool("usbgadget.autostart", usbgadget_autostart_set, &autostart, NULL);
+		/* We are already at latest initcall level, yet board code
+		 * may want to set this variable without resorting to scripts.
+		 * Check autostart manually here, so we don't miss it.
+		 */
+		if (autostart)
+			globalvar_set("usbgadget.autostart", "1");
+	}
 	return 0;
 }
 postenvironment_initcall(usbgadget_autostart_init);
diff --git a/include/usb/gadget-multi.h b/include/usb/gadget-multi.h
index 2d8d7533a816..e67ca165c18b 100644
--- a/include/usb/gadget-multi.h
+++ b/include/usb/gadget-multi.h
@@ -3,6 +3,7 @@
 #ifndef __USB_GADGET_MULTI_H
 #define __USB_GADGET_MULTI_H
 
+#include <linux/types.h>
 #include <usb/fastboot.h>
 #include <usb/dfu.h>
 #include <usb/usbserial.h>
@@ -36,4 +37,6 @@ struct usbgadget_funcs {
 
 int usbgadget_register(const struct usbgadget_funcs *funcs);
 
+void usbgadget_autostart(bool enable);
+
 #endif /* __USB_GADGET_MULTI_H */
-- 
2.30.2

Re: [PATCH] usbgadget: autostart: add usbgadget_autostart helper for board code

[Sascha Hauer]

Hi Ahmad,

On Mon, Aug 15, 2022 at 11:17:33AM +0200, Ahmad Fatoum wrote:
> barebox will rerun a setter if a nv variable changes its global variable's
> default. If setter is accessing other nv variables though, the setter
> will not rerun when they change. For this reason, usbgadget_autostart_init()
> must be registered at postenvironment_initcall() level, but this makes
> the variable only reliably usable from the shell as there's no later
> initcall level than postenvironment_initcall() for board code to run at.
> 
> Add a new usbgadget_autostart(boot enable) function that board code can
> use instead.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/usbgadget.c         | 15 ++++++++++++++-
>  include/usb/gadget-multi.h |  3 +++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/common/usbgadget.c b/common/usbgadget.c
> index 2ec6d9226cca..161fd16fedb7 100644
> --- a/common/usbgadget.c
> +++ b/common/usbgadget.c
> @@ -121,6 +121,12 @@ static int usbgadget_autostart_set(struct param_d *param, void *ctx)
>  	return err;
>  }
>  
> +void usbgadget_autostart(bool enable)
> +{
> +	globalvar_set("usbgadget.autostart", enable ? "1" : "0");
> +	autostart = enable;
> +}
> +
>  static int usbgadget_globalvars_init(void)
>  {
>  	globalvar_add_simple_bool("usbgadget.acm", &acm);
> @@ -132,8 +138,15 @@ device_initcall(usbgadget_globalvars_init);
>  
>  static int usbgadget_autostart_init(void)
>  {
> -	if (IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART))
> +	if (IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART)) {
>  		globalvar_add_bool("usbgadget.autostart", usbgadget_autostart_set, &autostart, NULL);
> +		/* We are already at latest initcall level, yet board code
> +		 * may want to set this variable without resorting to scripts.
> +		 * Check autostart manually here, so we don't miss it.
> +		 */
> +		if (autostart)
> +			globalvar_set("usbgadget.autostart", "1");
> +	}
>  	return 0;
>  }

The code is a bit hard to follow with the approach of starting the
gadget exclusively from the setter function of global.usbgadget.autostart

Can we go with the following approach? It moves the starting of the usb
gadget to a function that first checks if all prerequisites are
fulfilled to start the gadget and then starts it if desired.

Sascha

--------------------------8<---------------------------------

>From e246fc934cbd0423cb8d98353f8cd747f875011d Mon Sep 17 00:00:00 2001
From: Ahmad Fatoum <a.fatoum@pengutronix.de>
Date: Mon, 15 Aug 2022 11:17:33 +0200
Subject: [PATCH] usbgadget: autostart: add usbgadget_autostart() for board
 code

global.usbgadget.autostart is registered at postenvironment level
initcall, so changing its value before that doesn't have the desired
effect of automatically starting the usb gadget.

Refactor the code so that we have a usbgadget_do_autostart() function
that first checks if it should run at all and that it's the right time
to run. This function can be called at any time and based on that
implement usbgadget_autostart() to let board code be able to trigger
the autorun functionality.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/usbgadget.c         | 37 ++++++++++++++++++++++++++++++++-----
 include/usb/gadget-multi.h |  3 +++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/common/usbgadget.c b/common/usbgadget.c
index 2ec6d9226c..7291fbf4d5 100644
--- a/common/usbgadget.c
+++ b/common/usbgadget.c
@@ -20,6 +20,7 @@
 #include <system-partitions.h>
 
 static int autostart;
+static int nv_loaded;
 static int acm;
 static char *dfu_function;
 
@@ -98,13 +99,20 @@ err:
 	return ret;
 }
 
-static int usbgadget_autostart_set(struct param_d *param, void *ctx)
+static int usbgadget_do_autostart(void)
 {
 	struct usbgadget_funcs funcs = {};
 	static bool started;
 	int err;
 
-	if (!autostart || started)
+	if (!IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART))
+		return 0;
+
+	/*
+	 * We want to run this function exactly once when the
+	 * environment is loaded and autostart is requested
+	 */
+	if (!nv_loaded || !autostart || started)
 		return 0;
 
 	if (get_fastboot_bbu())
@@ -121,19 +129,38 @@ static int usbgadget_autostart_set(struct param_d *param, void *ctx)
 	return err;
 }
 
+static int usbgadget_autostart_set(struct param_d *param, void *ctx)
+{
+	usbgadget_do_autostart();
+
+	return 0;
+}
+
+void usbgadget_autostart(bool enable)
+{
+	autostart = enable;
+
+	usbgadget_do_autostart();
+}
+
 static int usbgadget_globalvars_init(void)
 {
 	globalvar_add_simple_bool("usbgadget.acm", &acm);
 	globalvar_add_simple_string("usbgadget.dfu_function", &dfu_function);
+	if (IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART))
+		globalvar_add_bool("usbgadget.autostart", usbgadget_autostart_set,
+				   &autostart, NULL);
 
 	return 0;
 }
-device_initcall(usbgadget_globalvars_init);
+coredevice_initcall(usbgadget_globalvars_init);
 
 static int usbgadget_autostart_init(void)
 {
-	if (IS_ENABLED(CONFIG_USB_GADGET_AUTOSTART))
-		globalvar_add_bool("usbgadget.autostart", usbgadget_autostart_set, &autostart, NULL);
+	nv_loaded = true;
+
+	usbgadget_do_autostart();
+
 	return 0;
 }
 postenvironment_initcall(usbgadget_autostart_init);
diff --git a/include/usb/gadget-multi.h b/include/usb/gadget-multi.h
index 2d8d7533a8..e67ca165c1 100644
--- a/include/usb/gadget-multi.h
+++ b/include/usb/gadget-multi.h
@@ -3,6 +3,7 @@
 #ifndef __USB_GADGET_MULTI_H
 #define __USB_GADGET_MULTI_H
 
+#include <linux/types.h>
 #include <usb/fastboot.h>
 #include <usb/dfu.h>
 #include <usb/usbserial.h>
@@ -36,4 +37,6 @@ struct usbgadget_funcs {
 
 int usbgadget_register(const struct usbgadget_funcs *funcs);
 
+void usbgadget_autostart(bool enable);
+
 #endif /* __USB_GADGET_MULTI_H */
-- 
2.30.2

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] usbgadget: autostart: add usbgadget_autostart helper for board code

[Ahmad Fatoum]

Hello Sascha,

On 16.08.22 09:57, Sascha Hauer wrote:
> The code is a bit hard to follow with the approach of starting the
> gadget exclusively from the setter function of global.usbgadget.autostart
> 
> Can we go with the following approach? It moves the starting of the usb
> gadget to a function that first checks if all prerequisites are
> fulfilled to start the gadget and then starts it if desired.

Just tested it and works for me. Please apply.

Thanks!
Ahmad


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] usbgadget: autostart: add usbgadget_autostart helper for board code

[Sascha Hauer]

On Wed, Aug 17, 2022 at 08:30:10AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 16.08.22 09:57, Sascha Hauer wrote:
> > The code is a bit hard to follow with the approach of starting the
> > gadget exclusively from the setter function of global.usbgadget.autostart
> > 
> > Can we go with the following approach? It moves the starting of the usb
> > gadget to a function that first checks if all prerequisites are
> > fulfilled to start the gadget and then starts it if desired.
> 
> Just tested it and works for me. Please apply.

Did that.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |