* [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
@ 2020-03-10 13:20 Andy Shevchenko
2020-03-10 13:20 ` [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 13:20 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: Andy Shevchenko
Compiler is not happy about using ARRAY_SIZE() in comparison to smaller type:
CC drivers/tty/serial/serial_core.o
.../serial_core.c: In function ‘uart_try_toggle_sysrq’:
.../serial_core.c:3222:24: warning: comparison is always false due to limited range of data type [-Wtype-limits]
3222 | if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
| ^
Looking at the code it appears that there is an additional weirdness,
i.e. use ARRAY_SIZE() against simple string literal. Yes, the idea probably
was to allow '\0' in the sequence, but it's impractical: kernel configuration
won't accept it to begin with followed by a comment about '\0' before
comparison in question.
Drop all these by switching to strlen() and convert code accordingly.
Fixes: 68af43173d3f ("serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/serial_core.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index aec98db45406..ec3b833e9f22 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
*/
static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
{
- if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
+ int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
+
+ if (!sysrq_toggle_seq_len)
return false;
BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= U8_MAX);
@@ -3218,8 +3220,7 @@ static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
return false;
}
- /* Without the last \0 */
- if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
+ if (++port->sysrq_seq < sysrq_toggle_seq_len) {
port->sysrq = jiffies + SYSRQ_TIMEOUT;
return true;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled
2020-03-10 13:20 [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
@ 2020-03-10 13:20 ` Andy Shevchenko
2020-03-10 14:40 ` Dmitry Safonov
2020-03-10 13:20 ` [PATCH v1 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 13:20 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: Andy Shevchenko
It is useful to see on the serial console the magic sequence itself
to enable SysRq without rummaging source code.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/serial_core.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ec3b833e9f22..e3f2afc15ad4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3191,8 +3191,11 @@ static const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
static void uart_sysrq_on(struct work_struct *w)
{
+ int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
+
sysrq_toggle_support(1);
- pr_info("SysRq is enabled by magic sequence on serial\n");
+ pr_info("SysRq is enabled by magic sequence '%*pE' on serial\n",
+ sysrq_toggle_seq_len, sysrq_toggle_seq);
}
static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 3/4] serial: core: Use uart_console() helper in SysRq code
2020-03-10 13:20 [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
2020-03-10 13:20 ` [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
@ 2020-03-10 13:20 ` Andy Shevchenko
2020-03-10 14:43 ` Dmitry Safonov
2020-03-10 13:20 ` [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 13:20 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: Andy Shevchenko
Use uart_console() helper in SysRq code instead of open coded variant.
This eliminates the conditional entirely for SERIAL_CORE_CONSOLE=n case.
While here, refactor the conditional to be more compact.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/serial_core.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e3f2afc15ad4..e3f8e641e3a7 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3315,14 +3315,12 @@ int uart_handle_break(struct uart_port *port)
if (port->handle_break)
port->handle_break(port);
- if (port->has_sysrq) {
- if (port->cons && port->cons->index == port->line) {
- if (!port->sysrq) {
- port->sysrq = jiffies + SYSRQ_TIMEOUT;
- return 1;
- }
- port->sysrq = 0;
+ if (port->has_sysrq && uart_console(port)) {
+ if (!port->sysrq) {
+ port->sysrq = jiffies + SYSRQ_TIMEOUT;
+ return 1;
}
+ port->sysrq = 0;
}
if (port->flags & UPF_SAK)
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()
2020-03-10 13:20 [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
2020-03-10 13:20 ` [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
2020-03-10 13:20 ` [PATCH v1 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
@ 2020-03-10 13:20 ` Andy Shevchenko
2020-03-10 14:48 ` Dmitry Safonov
2020-03-10 14:38 ` [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Dmitry Safonov
2020-03-10 17:06 ` Dmitry Safonov
4 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 13:20 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-serial, Dmitry Safonov; +Cc: Andy Shevchenko
Refactor uart_unlock_and_check_sysrq() to:
- explicitly show that we release a port lock which makes
static analyzers happy:
CHECK drivers/tty/serial/serial_core.c
.../serial_core.c:3290:17: warning: context imbalance in 'uart_unlock_and_check_sysrq' - unexpected unlock
- use flags instead of irqflags to avoid confusion with IRQ flags
- provide one return point
- be more compact
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/serial_core.c | 22 ++++++++++------------
include/linux/serial_core.h | 3 +--
2 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e3f8e641e3a7..ee51cf87e409 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3286,22 +3286,20 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
}
EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
-void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
+void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
+__releases(&port->lock)
{
int sysrq_ch;
- if (!port->has_sysrq) {
- spin_unlock_irqrestore(&port->lock, irqflags);
- return;
+ if (port->has_sysrq) {
+ sysrq_ch = port->sysrq_ch;
+ port->sysrq_ch = 0;
+ spin_unlock_irqrestore(&port->lock, flags);
+ if (sysrq_ch)
+ handle_sysrq(sysrq_ch);
+ } else {
+ spin_unlock_irqrestore(&port->lock, flags);
}
-
- sysrq_ch = port->sysrq_ch;
- port->sysrq_ch = 0;
-
- spin_unlock_irqrestore(&port->lock, irqflags);
-
- if (sysrq_ch)
- handle_sysrq(sysrq_ch);
}
EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 7a025042e0bb..cab896c60e35 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -447,8 +447,7 @@ extern void uart_insert_char(struct uart_port *port, unsigned int status,
extern int uart_handle_sysrq_char(struct uart_port *port, unsigned int ch);
extern int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch);
-extern void uart_unlock_and_check_sysrq(struct uart_port *port,
- unsigned long irqflags);
+extern void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags);
extern int uart_handle_break(struct uart_port *port);
/*
--
2.25.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 13:20 [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
` (2 preceding siblings ...)
2020-03-10 13:20 ` [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
@ 2020-03-10 14:38 ` Dmitry Safonov
2020-03-10 14:57 ` Andy Shevchenko
2020-03-10 17:06 ` Dmitry Safonov
4 siblings, 1 reply; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 14:38 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial
Hi Andy,
thanks for the patch,
On 3/10/20 1:20 PM, Andy Shevchenko wrote:
[..]> @@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work,
uart_sysrq_on);
> */
> static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> {
> - if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
> + int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
> +
> + if (!sysrq_toggle_seq_len)
> return false;
Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
is pressed. It's not very frequent moment surely, but..
Could you try
: int sysrq_toggle_seq_len = ARRAY_SIZE(sysrq_toggle_seq);
:
: if (sysrq_toggle_seq_len <= 1)
: return false;
: /* ... */
: port->sysrq_seq++;
: if (port->sysrq_seq + 1 < sysrq_toggle_seq_len) {
if this will shut the warning instead?
BTW, is this gcc 10 you see the warning with?
I have gcc (GCC) 9.2.0 and I don't see a warning with/without the config
string.
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled
2020-03-10 13:20 ` [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
@ 2020-03-10 14:40 ` Dmitry Safonov
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 14:40 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial
On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> It is useful to see on the serial console the magic sequence itself
> to enable SysRq without rummaging source code.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
> ---
> drivers/tty/serial/serial_core.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index ec3b833e9f22..e3f2afc15ad4 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3191,8 +3191,11 @@ static const char sysrq_toggle_seq[] = CONFIG_MAGIC_SYSRQ_SERIAL_SEQUENCE;
>
> static void uart_sysrq_on(struct work_struct *w)
> {
> + int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
> +
> sysrq_toggle_support(1);
> - pr_info("SysRq is enabled by magic sequence on serial\n");
> + pr_info("SysRq is enabled by magic sequence '%*pE' on serial\n",
> + sysrq_toggle_seq_len, sysrq_toggle_seq);
> }
> static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
>
>
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/4] serial: core: Use uart_console() helper in SysRq code
2020-03-10 13:20 ` [PATCH v1 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
@ 2020-03-10 14:43 ` Dmitry Safonov
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 14:43 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial
On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> Use uart_console() helper in SysRq code instead of open coded variant.
> This eliminates the conditional entirely for SERIAL_CORE_CONSOLE=n case.
> While here, refactor the conditional to be more compact.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Thanks,
Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
> ---
> drivers/tty/serial/serial_core.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index e3f2afc15ad4..e3f8e641e3a7 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3315,14 +3315,12 @@ int uart_handle_break(struct uart_port *port)
> if (port->handle_break)
> port->handle_break(port);
>
> - if (port->has_sysrq) {
> - if (port->cons && port->cons->index == port->line) {
> - if (!port->sysrq) {
> - port->sysrq = jiffies + SYSRQ_TIMEOUT;
> - return 1;
> - }
> - port->sysrq = 0;
> + if (port->has_sysrq && uart_console(port)) {
> + if (!port->sysrq) {
> + port->sysrq = jiffies + SYSRQ_TIMEOUT;
> + return 1;
> }
> + port->sysrq = 0;
> }
>
> if (port->flags & UPF_SAK)
>
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()
2020-03-10 13:20 ` [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
@ 2020-03-10 14:48 ` Dmitry Safonov
2020-03-10 15:30 ` Andy Shevchenko
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 14:48 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial
On 3/10/20 1:20 PM, Andy Shevchenko wrote:
[..]
> @@ -3286,22 +3286,20 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
> }
> EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
>
> -void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
> +void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
> +__releases(&port->lock)
> {
> int sysrq_ch;
Probably, you could move it inside the condition it's used.
Though, I wonder why you decided to rearrange the code.
Otherwise, LGTM.
>
> - if (!port->has_sysrq) {
> - spin_unlock_irqrestore(&port->lock, irqflags);
> - return;
> + if (port->has_sysrq) {
> + sysrq_ch = port->sysrq_ch;
> + port->sysrq_ch = 0;
> + spin_unlock_irqrestore(&port->lock, flags);
> + if (sysrq_ch)
> + handle_sysrq(sysrq_ch);
> + } else {
> + spin_unlock_irqrestore(&port->lock, flags);
> }
> -
> - sysrq_ch = port->sysrq_ch;
> - port->sysrq_ch = 0;
> -
> - spin_unlock_irqrestore(&port->lock, irqflags);
> -
> - if (sysrq_ch)
> - handle_sysrq(sysrq_ch);
> }
> EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
Thanks,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 14:38 ` [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Dmitry Safonov
@ 2020-03-10 14:57 ` Andy Shevchenko
2020-03-10 15:33 ` Andy Shevchenko
2020-03-10 15:57 ` Dmitry Safonov
0 siblings, 2 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 14:57 UTC (permalink / raw)
To: Dmitry Safonov; +Cc: Greg Kroah-Hartman, linux-serial
On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> [..]> @@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work,
> uart_sysrq_on);
> > */
> > static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> > {
> > - if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
> > + int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
> > +
> > + if (!sysrq_toggle_seq_len)
> > return false;
>
> Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
> is pressed. It's not very frequent moment surely, but..
I really don't like ARRAY_SIZE() against plain strings.
This will use \0 inclusively and confuse the understanding the code.
> Could you try
I even can tell you w/o trying, that it will fix this (yes, it does),
but see above.
> : int sysrq_toggle_seq_len = ARRAY_SIZE(sysrq_toggle_seq);
> :
> : if (sysrq_toggle_seq_len <= 1)
> : return false;
> : /* ... */
> : port->sysrq_seq++;
> : if (port->sysrq_seq + 1 < sysrq_toggle_seq_len) {
>
> if this will shut the warning instead?
> BTW, is this gcc 10 you see the warning with?
> I have gcc (GCC) 9.2.0 and I don't see a warning with/without the config
> string.
gcc (Debian 9.2.1-30) 9.2.1 20200224
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq()
2020-03-10 14:48 ` Dmitry Safonov
@ 2020-03-10 15:30 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 15:30 UTC (permalink / raw)
To: Dmitry Safonov; +Cc: Greg Kroah-Hartman, linux-serial
On Tue, Mar 10, 2020 at 02:48:51PM +0000, Dmitry Safonov wrote:
> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> [..]
> > @@ -3286,22 +3286,20 @@ int uart_prepare_sysrq_char(struct uart_port *port, unsigned int ch)
> > }
> > EXPORT_SYMBOL_GPL(uart_prepare_sysrq_char);
> >
> > -void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long irqflags)
> > +void uart_unlock_and_check_sysrq(struct uart_port *port, unsigned long flags)
> > +__releases(&port->lock)
> > {
> > int sysrq_ch;
>
> Probably, you could move it inside the condition it's used.
I can do this.
> Though, I wonder why you decided to rearrange the code.
I hope commit message sheds a light on this. Main reason to (easily) see
how locks are being maintained.
> Otherwise, LGTM.
Thanks, I will send v2 when we get settlement on patch 1.
> > - if (!port->has_sysrq) {
> > - spin_unlock_irqrestore(&port->lock, irqflags);
> > - return;
> > + if (port->has_sysrq) {
> > + sysrq_ch = port->sysrq_ch;
> > + port->sysrq_ch = 0;
> > + spin_unlock_irqrestore(&port->lock, flags);
> > + if (sysrq_ch)
> > + handle_sysrq(sysrq_ch);
> > + } else {
> > + spin_unlock_irqrestore(&port->lock, flags);
> > }
> > -
> > - sysrq_ch = port->sysrq_ch;
> > - port->sysrq_ch = 0;
> > -
> > - spin_unlock_irqrestore(&port->lock, irqflags);
> > -
> > - if (sysrq_ch)
> > - handle_sysrq(sysrq_ch);
> > }
> > EXPORT_SYMBOL_GPL(uart_unlock_and_check_sysrq);
>
> Thanks,
> Dmitry
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 14:57 ` Andy Shevchenko
@ 2020-03-10 15:33 ` Andy Shevchenko
2020-03-10 15:57 ` Dmitry Safonov
1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 15:33 UTC (permalink / raw)
To: Dmitry Safonov; +Cc: Greg Kroah-Hartman, linux-serial
On Tue, Mar 10, 2020 at 04:57:06PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
> > On 3/10/20 1:20 PM, Andy Shevchenko wrote:
...
> > BTW, is this gcc 10 you see the warning with?
> > I have gcc (GCC) 9.2.0 and I don't see a warning with/without the config
> > string.
>
> gcc (Debian 9.2.1-30) 9.2.1 20200224
I think it will be even on older versions.
I usually run build with `make O=... W=1 C=1 CF=-D__CHECK_ENDIAN__ -j64`.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 14:57 ` Andy Shevchenko
2020-03-10 15:33 ` Andy Shevchenko
@ 2020-03-10 15:57 ` Dmitry Safonov
2020-03-10 16:48 ` Andy Shevchenko
1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 15:57 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Greg Kroah-Hartman, linux-serial
Hi Andy,
On 3/10/20 2:57 PM, Andy Shevchenko wrote:
> On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
>> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
>> [..]> @@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work,
>> uart_sysrq_on);
>>> */
>>> static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
>>> {
>>> - if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
>>> + int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
>>> +
>>> + if (!sysrq_toggle_seq_len)
>>> return false;
>>
>> Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
>> is pressed. It's not very frequent moment surely, but..
>
> I really don't like ARRAY_SIZE() against plain strings.
> This will use \0 inclusively and confuse the understanding the code.
I still feel a bit awkward that handler will run strlen() for something
that could be done compile-time, but I won't insist.
Thanks again for looking into warning,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 15:57 ` Dmitry Safonov
@ 2020-03-10 16:48 ` Andy Shevchenko
2020-03-10 17:06 ` Dmitry Safonov
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2020-03-10 16:48 UTC (permalink / raw)
To: Dmitry Safonov; +Cc: Greg Kroah-Hartman, linux-serial
On Tue, Mar 10, 2020 at 03:57:17PM +0000, Dmitry Safonov wrote:
> Hi Andy,
>
> On 3/10/20 2:57 PM, Andy Shevchenko wrote:
> > On Tue, Mar 10, 2020 at 02:38:48PM +0000, Dmitry Safonov wrote:
> >> On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> >> [..]> @@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work,
> >> uart_sysrq_on);
> >>> */
> >>> static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> >>> {
> >>> - if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
> >>> + int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
> >>> +
> >>> + if (!sysrq_toggle_seq_len)
> >>> return false;
> >>
> >> Eh, I wanted to avoid the strlen() call in runtime for every time sysrq
> >> is pressed. It's not very frequent moment surely, but..
> >
> > I really don't like ARRAY_SIZE() against plain strings.
> > This will use \0 inclusively and confuse the understanding the code.
>
> I still feel a bit awkward that handler will run strlen() for something
> that could be done compile-time, but I won't insist.
Have you seen to the assembly?
For me it seems that GCC is clever enough to precalc length for constant
literals.
With default string (empty) there is no uart_sysrq_on() at all in the code.
With "pqr" I see this function and in particular:
621: be 03 00 00 00 mov $0x3,%esi
With "pqrst"
621: be 05 00 00 00 mov $0x5,%esi
(Note, I compiled entire series, that's why uart_sysrq_on() has strlen() in)
> Thanks again for looking into warning,
You are welcome.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 16:48 ` Andy Shevchenko
@ 2020-03-10 17:06 ` Dmitry Safonov
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 17:06 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Greg Kroah-Hartman, linux-serial
On 3/10/20 4:48 PM, Andy Shevchenko wrote:
>
> Have you seen to the assembly?
>
> For me it seems that GCC is clever enough to precalc length for constant
> literals.
>
> With default string (empty) there is no uart_sysrq_on() at all in the code.
>
> With "pqr" I see this function and in particular:
> 621: be 03 00 00 00 mov $0x3,%esi
>
> With "pqrst"
>
> 621: be 05 00 00 00 mov $0x5,%esi
>
> (Note, I compiled entire series, that's why uart_sysrq_on() has strlen() in)
Ah, that solves all worries I have.
Thanks again,
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence
2020-03-10 13:20 [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
` (3 preceding siblings ...)
2020-03-10 14:38 ` [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Dmitry Safonov
@ 2020-03-10 17:06 ` Dmitry Safonov
4 siblings, 0 replies; 15+ messages in thread
From: Dmitry Safonov @ 2020-03-10 17:06 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, linux-serial
On 3/10/20 1:20 PM, Andy Shevchenko wrote:
> Compiler is not happy about using ARRAY_SIZE() in comparison to smaller type:
>
> CC drivers/tty/serial/serial_core.o
> .../serial_core.c: In function ‘uart_try_toggle_sysrq’:
> .../serial_core.c:3222:24: warning: comparison is always false due to limited range of data type [-Wtype-limits]
> 3222 | if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
> | ^
>
> Looking at the code it appears that there is an additional weirdness,
> i.e. use ARRAY_SIZE() against simple string literal. Yes, the idea probably
> was to allow '\0' in the sequence, but it's impractical: kernel configuration
> won't accept it to begin with followed by a comment about '\0' before
> comparison in question.
>
> Drop all these by switching to strlen() and convert code accordingly.
>
> Fixes: 68af43173d3f ("serial/sysrq: Add MAGIC_SYSRQ_SERIAL_SEQUENCE")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>
Thanks!
> ---
> drivers/tty/serial/serial_core.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index aec98db45406..ec3b833e9f22 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3209,7 +3209,9 @@ static DECLARE_WORK(sysrq_enable_work, uart_sysrq_on);
> */
> static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> {
> - if (ARRAY_SIZE(sysrq_toggle_seq) <= 1)
> + int sysrq_toggle_seq_len = strlen(sysrq_toggle_seq);
> +
> + if (!sysrq_toggle_seq_len)
> return false;
>
> BUILD_BUG_ON(ARRAY_SIZE(sysrq_toggle_seq) >= U8_MAX);
> @@ -3218,8 +3220,7 @@ static bool uart_try_toggle_sysrq(struct uart_port *port, unsigned int ch)
> return false;
> }
>
> - /* Without the last \0 */
> - if (++port->sysrq_seq < (ARRAY_SIZE(sysrq_toggle_seq) - 1)) {
> + if (++port->sysrq_seq < sysrq_toggle_seq_len) {
> port->sysrq = jiffies + SYSRQ_TIMEOUT;
> return true;
> }
>
--
Dima
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2020-03-10 17:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 13:20 [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Andy Shevchenko
2020-03-10 13:20 ` [PATCH v1 2/4] serial: core: Print escaped SysRq Magic sequence if enabled Andy Shevchenko
2020-03-10 14:40 ` Dmitry Safonov
2020-03-10 13:20 ` [PATCH v1 3/4] serial: core: Use uart_console() helper in SysRq code Andy Shevchenko
2020-03-10 14:43 ` Dmitry Safonov
2020-03-10 13:20 ` [PATCH v1 4/4] serial: core: Refactor uart_unlock_and_check_sysrq() Andy Shevchenko
2020-03-10 14:48 ` Dmitry Safonov
2020-03-10 15:30 ` Andy Shevchenko
2020-03-10 14:38 ` [PATCH v1 1/4] serial: core: Use string length for SysRq magic sequence Dmitry Safonov
2020-03-10 14:57 ` Andy Shevchenko
2020-03-10 15:33 ` Andy Shevchenko
2020-03-10 15:57 ` Dmitry Safonov
2020-03-10 16:48 ` Andy Shevchenko
2020-03-10 17:06 ` Dmitry Safonov
2020-03-10 17:06 ` Dmitry Safonov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.