Re: [PATCH v2] earlycon: Fix __earlycon_table stride... again

[Randy Dunlap]

On 03/14/2018 06:48 PM, Daniel Kurtz wrote:
> Commit 470ca0de69fe ("serial: earlycon: Enable earlycon without command
> line param") added EARLYCON_TABLE().
> 
> Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") referenced
> commit 07fca0e57fca92 ("tracing: Properly align linker defined symbols")
> and tried to fix EARLYCON_TABLE() using __aligned(32).
> 
> However, the fix copied just works around compiler behavior.  In fact, it
> was subsequently re-implemented for tracing using a table of pointers in
> commit 3d56e331b653 ("tracing: Replace syscall_meta_data struct array with
> pointer array"), commit 654986462939 ("tracepoints: Fix section alignment
> using pointer array") and commit e4a9ea5ee7c8 ("tracing: Replace
> trace_event struct array with pointer array").
> 
> Let's do the same "array of pointers to structs" approach for
> EARLYCON_TABLE.

Please describe what the problem is instead of just what the fix is.
Thanks.

> Signed-off-by: Daniel Kurtz <djkurtz@xxxxxxxxxxxx>
> ---
> Changes since v1:
>  * Change __earlycon_table to an array of pointers.
> 
> Note: I do not have a convenient system to test the of/fdt.c part of this
> patch, so would apreciate help verifying it.  Thanks!
> 
>  drivers/of/fdt.c                  |  7 +++++--
>  drivers/tty/serial/earlycon.c     |  6 ++++--
>  include/asm-generic/vmlinux.lds.h |  2 +-
>  include/linux/serial_core.h       | 21 ++++++++++++++-------
>  4 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 84aa9d676375..6da20b9688f7 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
>  	int offset;
>  	const char *p, *q, *options = NULL;
>  	int l;
> -	const struct earlycon_id *match;
> +	const struct earlycon_id **p_match;
>  	const void *fdt = initial_boot_params;
>  
>  	offset = fdt_path_offset(fdt, "/chosen");
> @@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
>  		return 0;
>  	}
>  
> -	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> +	for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> +	     p_match++) {
> +		const struct earlycon_id *match = *p_match;
> +
>  		if (!match->compatible[0])
>  			continue;
>  
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a24278380fec..22683393a0f2 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
>   */
>  int __init setup_earlycon(char *buf)
>  {
> -	const struct earlycon_id *match;
> +	const struct earlycon_id **p_match;
>  
>  	if (!buf || !buf[0])
>  		return -EINVAL;
> @@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
>  	if (early_con.flags & CON_ENABLED)
>  		return -EALREADY;
>  
> -	for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> +	for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> +	     p_match++) {
> +		const struct earlycon_id *match = *p_match;
>  		size_t len = strlen(match->name);
>  
>  		if (strncmp(buf, match->name, len))
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 1ab0e520d6fc..e17de55c2542 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -179,7 +179,7 @@
>  #endif
>  
>  #ifdef CONFIG_SERIAL_EARLYCON
> -#define EARLYCON_TABLE() STRUCT_ALIGN();			\
> +#define EARLYCON_TABLE() . = ALIGN(8);				\
>  			 VMLINUX_SYMBOL(__earlycon_table) = .;	\
>  			 KEEP(*(__earlycon_table))		\
>  			 VMLINUX_SYMBOL(__earlycon_table_end) = .;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index b32df49a3bd5..93b7add47087 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -351,10 +351,10 @@ struct earlycon_id {
>  	char	name[16];
>  	char	compatible[128];
>  	int	(*setup)(struct earlycon_device *, const char *options);
> -} __aligned(32);
> +};
>  
> -extern const struct earlycon_id __earlycon_table[];
> -extern const struct earlycon_id __earlycon_table_end[];
> +extern const struct earlycon_id *__earlycon_table[];
> +extern const struct earlycon_id *__earlycon_table_end[];
>  
>  #if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
>  #define EARLYCON_USED_OR_UNUSED	__used
> @@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
>  #define EARLYCON_USED_OR_UNUSED	__maybe_unused
>  #endif
>  
> -#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
> -	static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name)	\
> -	     EARLYCON_USED_OR_UNUSED __section(__earlycon_table)	\
> +#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id)		\
> +	static const struct earlycon_id unique_id			\
> +	     EARLYCON_USED_OR_UNUSED __initdata				\
>  		= { .name = __stringify(_name),				\
>  		    .compatible = compat,				\
> -		    .setup = fn  }
> +		    .setup = fn  };					\
> +	static const struct earlycon_id EARLYCON_USED_OR_UNUSED		\
> +		__section(__earlycon_table)				\
> +		* const __PASTE(__p, unique_id) = &unique_id
> +
> +#define OF_EARLYCON_DECLARE(_name, compat, fn)				\
> +	_OF_EARLYCON_DECLARE(_name, compat, fn,				\
> +			     __UNIQUE_ID(__earlycon_##_name))
>  
>  #define EARLYCON_DECLARE(_name, fn)	OF_EARLYCON_DECLARE(_name, "", fn)
>  
> 


-- 
~Randy