[PATCH v2 1/7] console: Introduce console_start() and console_stop()

[PATCH v2 1/7] console: Introduce console_start() and console_stop()

[Andy Shevchenko]

In the future we would like to stop unused consoles and
also add a reference counting to avoid imbalanced calls
to ->start() and ->stop() in some cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 common/console.c  | 30 +++++++++++++++++++++++-------
 include/console.h |  3 +++
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/common/console.c b/common/console.c
index 3348436da6f7..1efbcc7672ce 100644
--- a/common/console.c
+++ b/common/console.c
@@ -114,13 +114,9 @@ static int console_setfile(int file, struct stdio_dev * dev)
 	case stdin:
 	case stdout:
 	case stderr:
-		/* Start new device */
-		if (dev->start) {
-			error = dev->start(dev);
-			/* If it's not started dont use it */
-			if (error < 0)
-				break;
-		}
+		error = console_start(file, dev);
+		if (error)
+			break;
 
 		/* Assign the new device (leaving the existing one started) */
 		stdio_devices[file] = dev;
@@ -310,6 +306,26 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
 #endif
 #endif /* CONIFIG_IS_ENABLED(CONSOLE_MUX) */
 
+int console_start(int file, struct stdio_dev *dev)
+{
+	int error;
+
+	/* Start new device */
+	if (dev->start) {
+		error = dev->start(dev);
+		/* If it's not started don't use it */
+		if (error < 0)
+			return error;
+	}
+	return 0;
+}
+
+void console_stop(int file, struct stdio_dev *dev)
+{
+	if (dev->stop)
+		dev->stop(dev);
+}
+
 /** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS *************************/
 
 int serial_printf(const char *fmt, ...)
diff --git a/include/console.h b/include/console.h
index 432f892b6cce..233ff323e1ee 100644
--- a/include/console.h
+++ b/include/console.h
@@ -8,6 +8,7 @@
 #define __CONSOLE_H
 
 #include <stdbool.h>
+#include <stdio_dev.h>
 #include <linux/errno.h>
 
 extern char console_buffer[];
@@ -15,6 +16,8 @@ extern char console_buffer[];
 /* common/console.c */
 int console_init_f(void);	/* Before relocation; uses the serial  stuff */
 int console_init_r(void);	/* After  relocation; uses the console stuff */
+int console_start(int file, struct stdio_dev *dev);
+void console_stop(int file, struct stdio_dev *dev);
 int console_assign(int file, const char *devname);	/* Assign the console */
 int ctrlc(void);
 int had_ctrlc(void);	/* have we had a Control-C since last clear? */
-- 
2.29.2

[PATCH v2 2/7] console: Keep ->start() and ->stop() balanced

[Andy Shevchenko]

There is no need to call ->start() for already started device. All the same,
there is no need to call ->stop() for devices still in use.

For now enforce this only for CONSOLE_MUX case.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 common/console.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/common/console.c b/common/console.c
index 1efbcc7672ce..b051c24e7389 100644
--- a/common/console.c
+++ b/common/console.c
@@ -174,6 +174,16 @@ static struct stdio_dev *tstcdev;
 struct stdio_dev **console_devices[MAX_FILES];
 int cd_count[MAX_FILES];
 
+static bool console_needs_handle(int file, struct stdio_dev *dev)
+{
+	int i;
+
+	for (i = 0; i < cd_count[file]; i++)
+		if (console_devices[file][i] == dev)
+			return false;
+	return true;
+}
+
 /*
  * This depends on tstc() always being called before getchar().
  * This is guaranteed to be true because this routine is called
@@ -272,6 +282,11 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
 }
 #endif
 #else
+static inline bool console_needs_handle(int file, struct stdio_dev *dev)
+{
+	return true;
+}
+
 static inline int console_getc(int file)
 {
 	return stdio_devices[file]->getc(stdio_devices[file]);
@@ -310,6 +325,9 @@ int console_start(int file, struct stdio_dev *dev)
 {
 	int error;
 
+	if (!console_needs_handle(file, dev))
+		return 0;
+
 	/* Start new device */
 	if (dev->start) {
 		error = dev->start(dev);
@@ -322,6 +340,9 @@ int console_start(int file, struct stdio_dev *dev)
 
 void console_stop(int file, struct stdio_dev *dev)
 {
+	if (!console_needs_handle(file, dev))
+		return;
+
 	if (dev->stop)
 		dev->stop(dev);
 }
-- 
2.29.2

[PATCH v2 3/7] IOMUX: move search_device() to console.h

[Andy Shevchenko]

search_device() is defined in console.c. Move its declaration
to an appropriate header file.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: new patch
 include/console.h | 2 ++
 include/iomux.h   | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/console.h b/include/console.h
index 233ff323e1ee..79f9534a9535 100644
--- a/include/console.h
+++ b/include/console.h
@@ -25,6 +25,8 @@ void clear_ctrlc(void);	/* clear the Control-C condition */
 int disable_ctrlc(int);	/* 1 to disable, 0 to enable Control-C detect */
 int confirm_yesno(void);        /*  1 if input is "y", "Y", "yes" or "YES" */
 
+struct stdio_dev *search_device(int flags, const char *name);
+
 #ifdef CONFIG_CONSOLE_RECORD
 /**
  * console_record_init() - set up the console recording buffers
diff --git a/include/iomux.h b/include/iomux.h
index e6e1097db5b2..da7ff697d218 100644
--- a/include/iomux.h
+++ b/include/iomux.h
@@ -26,6 +26,5 @@ extern int cd_count[MAX_FILES];
 
 int iomux_doenv(const int, const char *);
 void iomux_printdevs(const int);
-struct stdio_dev *search_device(int, const char *);
 
 #endif /* _IO_MUX_H */
-- 
2.29.2

[PATCH v2 4/7] IOMUX: Preserve console list if realloc() fails

[Andy Shevchenko]

It's realloc() 101 to avoid `foo = realloc(foo, ...);` call
due to getting a memory leak.

Actually it's not clear why realloc() has been used here.
If we shrink the array, the memcpy() overwrites it anyway
with the contents of a new array. If it becomes bigger,
same story.

Drop useless realloc() for good and thus preserve console list
in case of failed allocation.

Fixes: 16a28ef219c2 ("IOMUX: Add console multiplexing support.")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 common/iomux.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/common/iomux.c b/common/iomux.c
index 7cfd9f2e9162..cee5f266c86e 100644
--- a/common/iomux.c
+++ b/common/iomux.c
@@ -129,19 +129,10 @@ int iomux_doenv(const int console, const char *arg)
 		return 1;
 	} else {
 		/* Works even if console_devices[console] is NULL. */
-		console_devices[console] =
-			(struct stdio_dev **)realloc(console_devices[console],
-			cs_idx * sizeof(struct stdio_dev *));
-		if (console_devices[console] == NULL) {
-			free(cons_set);
-			return 1;
-		}
-		memcpy(console_devices[console], cons_set, cs_idx *
-			sizeof(struct stdio_dev *));
-
+		free(console_devices[console]);
+		console_devices[console] = cons_set;
 		cd_count[console] = cs_idx;
 	}
-	free(cons_set);
 	return 0;
 }
 #endif /* CONSOLE_MUX */
-- 
2.29.2

[PATCH v2 5/7] IOMUX: Refactor iomux_doenv() in order to increase readability

[Andy Shevchenko]

Refactor iomux_doenv() a bit in order to increase readability.
There is no change in code generation on x86.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 common/iomux.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/common/iomux.c b/common/iomux.c
index cee5f266c86e..51557d028029 100644
--- a/common/iomux.c
+++ b/common/iomux.c
@@ -45,15 +45,14 @@ int iomux_doenv(const int console, const char *arg)
 	i = 0;
 	temp = console_args;
 	for (;;) {
-		temp = strchr(temp, ',');
-		if (temp != NULL) {
-			i++;
-			temp++;
-			continue;
-		}
 		/* There's always one entry more than the number of commas. */
 		i++;
-		break;
+
+		temp = strchr(temp, ',');
+		if (temp == NULL)
+			break;
+
+		temp++;
 	}
 	start = (char **)malloc(i * sizeof(char *));
 	if (start == NULL) {
-- 
2.29.2

[PATCH v2 6/7] IOMUX: Drop indentation level by removing redundant 'else'

[Andy Shevchenko]

Obviously the following has unnecessary indentation level in 'else' branch.

	if (foo) {
		...
		return;
	} else {
		...
	}

Drop indentation level by removing redundant 'else'.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: no changes
 common/iomux.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/common/iomux.c b/common/iomux.c
index 51557d028029..8a063563aa27 100644
--- a/common/iomux.c
+++ b/common/iomux.c
@@ -126,12 +126,12 @@ int iomux_doenv(const int console, const char *arg)
 	if (cs_idx == 0) {
 		free(cons_set);
 		return 1;
-	} else {
-		/* Works even if console_devices[console] is NULL. */
-		free(console_devices[console]);
-		console_devices[console] = cons_set;
-		cd_count[console] = cs_idx;
 	}
+
+	/* Works even if console_devices[console] is NULL. */
+	free(console_devices[console]);
+	console_devices[console] = cons_set;
+	cd_count[console] = cs_idx;
 	return 0;
 }
 #endif /* CONSOLE_MUX */
-- 
2.29.2

[PATCH v2 7/7] IOMUX: Stop dropped consoles

[Andy Shevchenko]

When at some point environment shrinks we need to stop dropped devices.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: rebased to use console_stop()
 common/iomux.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/common/iomux.c b/common/iomux.c
index 8a063563aa27..4fe73c8cba8a 100644
--- a/common/iomux.c
+++ b/common/iomux.c
@@ -27,8 +27,8 @@ int iomux_doenv(const int console, const char *arg)
 {
 	char *console_args, *temp, **start;
 	int i, j, k, io_flag, cs_idx, repeat;
+	struct stdio_dev **cons_set, **old_set;
 	struct stdio_dev *dev;
-	struct stdio_dev **cons_set;
 
 	console_args = strdup(arg);
 	if (console_args == NULL)
@@ -128,10 +128,23 @@ int iomux_doenv(const int console, const char *arg)
 		return 1;
 	}
 
-	/* Works even if console_devices[console] is NULL. */
-	free(console_devices[console]);
+	old_set = console_devices[console];
+	repeat = cd_count[console];
+
 	console_devices[console] = cons_set;
 	cd_count[console] = cs_idx;
+
+	/* Stop dropped consoles */
+	for (i = 0; i < repeat; i++) {
+		for (j = 0; j < cs_idx; j++) {
+			if (old_set[i] == cons_set[j])
+				break;
+		}
+		if (j == cs_idx)
+			console_stop(console, old_set[i]);
+	}
+
+	free(old_set);
 	return 0;
 }
 #endif /* CONSOLE_MUX */
-- 
2.29.2

[PATCH v2 2/7] console: Keep ->start() and ->stop() balanced

[Andy Shevchenko]

On Thu, Dec 17, 2020 at 1:16 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> There is no need to call ->start() for already started device. All the same,

an already

> there is no need to call ->stop() for devices still in use.
>
> For now enforce this only for CONSOLE_MUX case.

now, enforce
the CONSOLE_MUX

...

> +static bool console_needs_handle(int file, struct stdio_dev *dev)
> +{
> +       int i;
> +
> +       for (i = 0; i < cd_count[file]; i++)
> +               if (console_devices[file][i] == dev)
> +                       return false;

Actually this is no-op. I realized it later on.
It misses the iteration over all files. And file argument to exclude
iteration over that specific file.

I will update this, but will also wait a couple of days for other
comments against the series.

> +       return true;
> +}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/7] console: Introduce console_start() and console_stop()

[Simon Glass]

Hi Andy,

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> In the future we would like to stop unused consoles and
> also add a reference counting to avoid imbalanced calls
> to ->start() and ->stop() in some cases.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  common/console.c  | 30 +++++++++++++++++++++++-------
>  include/console.h |  3 +++
>  2 files changed, 26 insertions(+), 7 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

But please see below

> diff --git a/common/console.c b/common/console.c
> index 3348436da6f7..1efbcc7672ce 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -114,13 +114,9 @@ static int console_setfile(int file, struct stdio_dev * dev)
>         case stdin:
>         case stdout:
>         case stderr:
> -               /* Start new device */
> -               if (dev->start) {
> -                       error = dev->start(dev);
> -                       /* If it's not started dont use it */
> -                       if (error < 0)
> -                               break;
> -               }
> +               error = console_start(file, dev);
> +               if (error)
> +                       break;
>
>                 /* Assign the new device (leaving the existing one started) */
>                 stdio_devices[file] = dev;
> @@ -310,6 +306,26 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
>  #endif
>  #endif /* CONIFIG_IS_ENABLED(CONSOLE_MUX) */
>
> +int console_start(int file, struct stdio_dev *dev)
> +{
> +       int error;
> +
> +       /* Start new device */
> +       if (dev->start) {
> +               error = dev->start(dev);
> +               /* If it's not started don't use it */
> +               if (error < 0)
> +                       return error;
> +       }
> +       return 0;
> +}
> +
> +void console_stop(int file, struct stdio_dev *dev)
> +{
> +       if (dev->stop)
> +               dev->stop(dev);
> +}
> +
>  /** U-Boot INITIAL CONSOLE-NOT COMPATIBLE FUNCTIONS *************************/
>
>  int serial_printf(const char *fmt, ...)
> diff --git a/include/console.h b/include/console.h
> index 432f892b6cce..233ff323e1ee 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -8,6 +8,7 @@
>  #define __CONSOLE_H
>
>  #include <stdbool.h>
> +#include <stdio_dev.h>
>  #include <linux/errno.h>
>
>  extern char console_buffer[];
> @@ -15,6 +16,8 @@ extern char console_buffer[];
>  /* common/console.c */
>  int console_init_f(void);      /* Before relocation; uses the serial  stuff */
>  int console_init_r(void);      /* After  relocation; uses the console stuff */
> +int console_start(int file, struct stdio_dev *dev);
> +void console_stop(int file, struct stdio_dev *dev);

These two need comments

Also I'd prefer to use sdev for the args, since we use dev for a
driver model device.

>  int console_assign(int file, const char *devname);     /* Assign the console */
>  int ctrlc(void);
>  int had_ctrlc(void);   /* have we had a Control-C since last clear? */
> --
> 2.29.2
>

Regards,
Simon

[PATCH v2 3/7] IOMUX: move search_device() to console.h

[Simon Glass]

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> search_device() is defined in console.c. Move its declaration
> to an appropriate header file.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  include/console.h | 2 ++
>  include/iomux.h   | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

>
> diff --git a/include/console.h b/include/console.h
> index 233ff323e1ee..79f9534a9535 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -25,6 +25,8 @@ void clear_ctrlc(void);       /* clear the Control-C condition */
>  int disable_ctrlc(int);        /* 1 to disable, 0 to enable Control-C detect */
>  int confirm_yesno(void);        /*  1 if input is "y", "Y", "yes" or "YES" */
>
> +struct stdio_dev *search_device(int flags, const char *name);
> +
>  #ifdef CONFIG_CONSOLE_RECORD
>  /**
>   * console_record_init() - set up the console recording buffers
> diff --git a/include/iomux.h b/include/iomux.h
> index e6e1097db5b2..da7ff697d218 100644
> --- a/include/iomux.h
> +++ b/include/iomux.h
> @@ -26,6 +26,5 @@ extern int cd_count[MAX_FILES];
>
>  int iomux_doenv(const int, const char *);
>  void iomux_printdevs(const int);
> -struct stdio_dev *search_device(int, const char *);

Please add a full function comment

Also consider renaming it to console_search_dev() or similar, since it
is in console.h

>
>  #endif /* _IO_MUX_H */
> --
> 2.29.2
>

Regards,
Simon

[PATCH v2 4/7] IOMUX: Preserve console list if realloc() fails

[Simon Glass]

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> It's realloc() 101 to avoid `foo = realloc(foo, ...);` call
> due to getting a memory leak.

Hmm I don't think I knew that...

>
> Actually it's not clear why realloc() has been used here.
> If we shrink the array, the memcpy() overwrites it anyway
> with the contents of a new array. If it becomes bigger,
> same story.
>
> Drop useless realloc() for good and thus preserve console list
> in case of failed allocation.
>
> Fixes: 16a28ef219c2 ("IOMUX: Add console multiplexing support.")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: no changes
>  common/iomux.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>


> diff --git a/common/iomux.c b/common/iomux.c
> index 7cfd9f2e9162..cee5f266c86e 100644
> --- a/common/iomux.c
> +++ b/common/iomux.c
> @@ -129,19 +129,10 @@ int iomux_doenv(const int console, const char *arg)
>                 return 1;
>         } else {
>                 /* Works even if console_devices[console] is NULL. */
> -               console_devices[console] =
> -                       (struct stdio_dev **)realloc(console_devices[console],
> -                       cs_idx * sizeof(struct stdio_dev *));
> -               if (console_devices[console] == NULL) {
> -                       free(cons_set);
> -                       return 1;
> -               }
> -               memcpy(console_devices[console], cons_set, cs_idx *
> -                       sizeof(struct stdio_dev *));
> -
> +               free(console_devices[console]);
> +               console_devices[console] = cons_set;
>                 cd_count[console] = cs_idx;
>         }
> -       free(cons_set);
>         return 0;
>  }
>  #endif /* CONSOLE_MUX */
> --
> 2.29.2
>

[PATCH v2 5/7] IOMUX: Refactor iomux_doenv() in order to increase readability

[Simon Glass]

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Refactor iomux_doenv() a bit in order to increase readability.
> There is no change in code generation on x86.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: no changes
>  common/iomux.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>


>
> diff --git a/common/iomux.c b/common/iomux.c
> index cee5f266c86e..51557d028029 100644
> --- a/common/iomux.c
> +++ b/common/iomux.c
> @@ -45,15 +45,14 @@ int iomux_doenv(const int console, const char *arg)
>         i = 0;
>         temp = console_args;
>         for (;;) {
> -               temp = strchr(temp, ',');
> -               if (temp != NULL) {

event better:

if (temp)


> -                       i++;
> -                       temp++;
> -                       continue;
> -               }
>                 /* There's always one entry more than the number of commas. */
>                 i++;
> -               break;
> +
> +               temp = strchr(temp, ',');
> +               if (temp == NULL)
> +                       break;
> +
> +               temp++;
>         }
>         start = (char **)malloc(i * sizeof(char *));
>         if (start == NULL) {
> --
> 2.29.2
>

[PATCH v2 6/7] IOMUX: Drop indentation level by removing redundant 'else'

[Simon Glass]

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Obviously the following has unnecessary indentation level in 'else' branch.
>
>         if (foo) {
>                 ...
>                 return;
>         } else {
>                 ...
>         }
>
> Drop indentation level by removing redundant 'else'.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: no changes
>  common/iomux.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 7/7] IOMUX: Stop dropped consoles

[Simon Glass]

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> When at some point environment shrinks we need to stop dropped devices.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: rebased to use console_stop()
>  common/iomux.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH v2 2/7] console: Keep ->start() and ->stop() balanced

[Simon Glass]

Hi Andy,

On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> There is no need to call ->start() for already started device. All the same,
> there is no need to call ->stop() for devices still in use.
>
> For now enforce this only for CONSOLE_MUX case.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: new patch
>  common/console.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/common/console.c b/common/console.c
> index 1efbcc7672ce..b051c24e7389 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -174,6 +174,16 @@ static struct stdio_dev *tstcdev;
>  struct stdio_dev **console_devices[MAX_FILES];
>  int cd_count[MAX_FILES];
>
> +static bool console_needs_handle(int file, struct stdio_dev *dev)

function comment - what does this do?

> +{
> +       int i;
> +
> +       for (i = 0; i < cd_count[file]; i++)
> +               if (console_devices[file][i] == dev)
> +                       return false;
> +       return true;
> +}
> +
>  /*
>   * This depends on tstc() always being called before getchar().
>   * This is guaranteed to be true because this routine is called
> @@ -272,6 +282,11 @@ static inline void console_doenv(int file, struct stdio_dev *dev)
>  }
>  #endif
>  #else
> +static inline bool console_needs_handle(int file, struct stdio_dev *dev)
> +{
> +       return true;
> +}
> +
>  static inline int console_getc(int file)
>  {
>         return stdio_devices[file]->getc(stdio_devices[file]);
> @@ -310,6 +325,9 @@ int console_start(int file, struct stdio_dev *dev)
>  {
>         int error;
>
> +       if (!console_needs_handle(file, dev))
> +               return 0;
> +
>         /* Start new device */
>         if (dev->start) {
>                 error = dev->start(dev);
> @@ -322,6 +340,9 @@ int console_start(int file, struct stdio_dev *dev)
>
>  void console_stop(int file, struct stdio_dev *dev)
>  {
> +       if (!console_needs_handle(file, dev))
> +               return;
> +
>         if (dev->stop)
>                 dev->stop(dev);
>  }
> --
> 2.29.2
>

Regards,
Simon

[PATCH v2 4/7] IOMUX: Preserve console list if realloc() fails

[Andy Shevchenko]

On Fri, Dec 18, 2020 at 07:29:19PM -0700, Simon Glass wrote:
> On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > It's realloc() 101 to avoid `foo = realloc(foo, ...);` call
> > due to getting a memory leak.
> 
> Hmm I don't think I knew that...

When you use the same variable for the source and destination in case of NULL
the source gone.

It's okay to have

	foo = bar;
	bar = realloc(bar, ...);
	if (bar == NULL)
	...do something with foo if needed...

But it seems it's not the case here.

> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 5/7] IOMUX: Refactor iomux_doenv() in order to increase readability

[Andy Shevchenko]

On Fri, Dec 18, 2020 at 07:29:21PM -0700, Simon Glass wrote:
> On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Refactor iomux_doenv() a bit in order to increase readability.
> > There is no change in code generation on x86.

...

> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks!

...

> event better:
> 
> if (temp)

I didn't get this. Can you elaborate what exactly you had in mind?
Because...

> > +               temp = strchr(temp, ',');
> > +               if (temp == NULL)

...here is a new code.

> > +                       break;
> > +
> > +               temp++;


Since you gave Rb tag I'll leave as is for now.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/7] IOMUX: Preserve console list if realloc() fails

[Simon Glass]

Hi Andy,

On Mon, 21 Dec 2020 at 05:00, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Dec 18, 2020 at 07:29:19PM -0700, Simon Glass wrote:
> > On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > It's realloc() 101 to avoid `foo = realloc(foo, ...);` call
> > > due to getting a memory leak.
> >
> > Hmm I don't think I knew that...
>
> When you use the same variable for the source and destination in case of NULL
> the source gone.
>
> It's okay to have
>
>         foo = bar;
>         bar = realloc(bar, ...);
>         if (bar == NULL)
>         ...do something with foo if needed...

Here is man malloc on this point:

If ptr is NULL, then  the  call  is  equivalent  to  mal?
       loc(size), for all values of size; if size is equal to zero, and ptr is
       not NULL, then the call is equivalent  to  free(ptr).

>
> But it seems it's not the case here.
>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Thanks!

Regards,
Simon

[PATCH v2 5/7] IOMUX: Refactor iomux_doenv() in order to increase readability

[Simon Glass]

Hi Andy,

On Mon, 21 Dec 2020 at 05:03, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Dec 18, 2020 at 07:29:21PM -0700, Simon Glass wrote:
> > On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > Refactor iomux_doenv() a bit in order to increase readability.
> > > There is no change in code generation on x86.
>
> ...
>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Thanks!
>
> ...
>
> > event better:
> >
> > if (temp)
>
> I didn't get this. Can you elaborate what exactly you had in mind?
> Because...
>
> > > +               temp = strchr(temp, ',');
> > > +               if (temp == NULL)
>
> ...here is a new code.

It would help if I wrote it for the new code. I just mean:

if (temp == NULL)

is better written in U-Boot as

if (!temp)


>
> > > +                       break;
> > > +
> > > +               temp++;
>
>
> Since you gave Rb tag I'll leave as is for now.

Yes that's fine.

Regards,
Simon

[PATCH v2 4/7] IOMUX: Preserve console list if realloc() fails

[Andy Shevchenko]

On Mon, Dec 21, 2020 at 09:47:03AM -0700, Simon Glass wrote:
> On Mon, 21 Dec 2020 at 05:00, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Dec 18, 2020 at 07:29:19PM -0700, Simon Glass wrote:
> > > On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > It's realloc() 101 to avoid `foo = realloc(foo, ...);` call
> > > > due to getting a memory leak.
> > >
> > > Hmm I don't think I knew that...
> >
> > When you use the same variable for the source and destination in case of NULL
> > the source gone.
> >
> > It's okay to have
> >
> >         foo = bar;
> >         bar = realloc(bar, ...);
> >         if (bar == NULL)
> >         ...do something with foo if needed...
> 
> Here is man malloc on this point:
> 
> If ptr is NULL, then  the  call  is  equivalent  to  mal?
>        loc(size), for all values of size; if size is equal to zero, and ptr is
>        not NULL, then the call is equivalent  to  free(ptr).

But it's about another case.
I'm talking about realloc() to fail.

	foo = realloc(foo, ...);

will effectively leak memory if foo is not saved previously somewhere.
And this is the case here.

For instance [1] is telling about the same:
  "Of course if you will write

    p = realloc(p, 2 * sizeof(int));

  ... if the function was unable to reallocate memory. In this case a memory
  leak will occur provided that initial value of the pointer p was not equal
  to NULL."

Really, it's 101 of realloc() usage.

[1]: https://stackoverflow.com/questions/57498538/does-realloc-mutate-its-arguments

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/7] IOMUX: Preserve console list if realloc() fails

[Simon Glass]

Hi Andy,

On Mon, 21 Dec 2020 at 10:16, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Dec 21, 2020 at 09:47:03AM -0700, Simon Glass wrote:
> > On Mon, 21 Dec 2020 at 05:00, Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Dec 18, 2020 at 07:29:19PM -0700, Simon Glass wrote:
> > > > On Wed, 16 Dec 2020 at 16:16, Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > >
> > > > > It's realloc() 101 to avoid `foo = realloc(foo, ...);` call
> > > > > due to getting a memory leak.
> > > >
> > > > Hmm I don't think I knew that...
> > >
> > > When you use the same variable for the source and destination in case of NULL
> > > the source gone.
> > >
> > > It's okay to have
> > >
> > >         foo = bar;
> > >         bar = realloc(bar, ...);
> > >         if (bar == NULL)
> > >         ...do something with foo if needed...
> >
> > Here is man malloc on this point:
> >
> > If ptr is NULL, then  the  call  is  equivalent  to  mal?
> >        loc(size), for all values of size; if size is equal to zero, and ptr is
> >        not NULL, then the call is equivalent  to  free(ptr).
>
> But it's about another case.
> I'm talking about realloc() to fail.
>
>         foo = realloc(foo, ...);
>
> will effectively leak memory if foo is not saved previously somewhere.
> And this is the case here.
>
> For instance [1] is telling about the same:
>   "Of course if you will write
>
>     p = realloc(p, 2 * sizeof(int));
>
>   ... if the function was unable to reallocate memory. In this case a memory
>   leak will occur provided that initial value of the pointer p was not equal
>   to NULL."
>
> Really, it's 101 of realloc() usage.

Oh I missed that it failed...OK

>
> [1]: https://stackoverflow.com/questions/57498538/does-realloc-mutate-its-arguments

Regards,
Simon