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

[Sascha Hauer <sha@pengutronix.de>]

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 |