linux-kernel - Re: [PATCH 02/58] serial: Move sysrq members above

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20191213071749.GA1750354@kroah.com>
Date:   Fri, 13 Dec 2019 08:17:49 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Dmitry Safonov <dima@...sta.com>
Cc:     linux-kernel@...r.kernel.org,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Jiri Slaby <jslaby@...e.com>,
        Vasiliy Khoruzhick <vasilykh@...sta.com>,
        linux-serial@...r.kernel.org
Subject: Re: [PATCH 02/58] serial: Move sysrq members above

On Fri, Dec 13, 2019 at 12:06:01AM +0000, Dmitry Safonov wrote:
> At the current place members those follow are:
> :	upf_t			flags;
> :	upstat_t		status;
> :	int			hw_stopped;
> :	unsigned int		mctrl;
> :	unsigned int		timeout;
> :	unsigned int		type;
> :	const struct uart_ops	*ops;
> 
> Together, they give (*ops) 8-byte align on 64-bit platforms.
> And `sysrq_ch` introduces 4-byte padding.
> 
> On the other side, above:
> :	struct device		*dev;
> :	unsigned char		hub6;
> :	unsigned char		suspended;
> :	unsigned char		unused[2];
> :	const char		*name;
> 
> Adds another 4-byte padding.
> 
> Moving sysrq members just before `hub6` allows to save 8 bytes
> per-uart_port on 64-bit platforms:
> On my gcc, x86_64 sizeof(struct uart_port) goes from 528 to 520.
> 
> Signed-off-by: Dmitry Safonov <dima@...sta.com>
> ---
>  include/linux/serial_core.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 2b78cc734719..bbbe57bf5163 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -161,11 +161,6 @@ struct uart_port {
>  	struct uart_icount	icount;			/* statistics */
>  
>  	struct console		*cons;			/* struct console, if any */
> -#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
> -	unsigned long		sysrq;			/* sysrq timeout */
> -	unsigned int		sysrq_ch;		/* char for sysrq */
> -#endif
> -
>  	/* flags must be updated while holding port mutex */
>  	upf_t			flags;
>  
> @@ -244,6 +239,12 @@ struct uart_port {
>  	resource_size_t		mapbase;		/* for ioremap */
>  	resource_size_t		mapsize;
>  	struct device		*dev;			/* parent device */
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
> +	unsigned long		sysrq;			/* sysrq timeout */
> +	unsigned int		sysrq_ch;		/* char for sysrq */
> +#endif

Let's just always have these 2 fields in the structure, no need for a
#ifdef at all, right?  There is no real "savings" from removing them (8
bytes for a structure that is maybe created 2-4 times per machine?)

That way it would keep any of this mess of if the SUPPORT_SYSRQ is
enabled or not to keep everything in sync.

thanks,

greg k-h