2022-09-17 10:09:39

by Maciej W. Rozycki

[permalink] [raw]
Subject: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing

A SERIAL_8250_16550A_VARIANTS configuration option has been recently
defined that lets one request the 8250 driver not to probe for 16550A
device features so as to reduce the driver's device startup time in
virtual machines.

Some actual hardware devices require these features to have been fully
determined however for their driver to work correctly, so define a flag
to let drivers request full 16550A feature probing on a device-by-device
basis if required regardless of the SERIAL_8250_16550A_VARIANTS option
setting chosen.

Signed-off-by: Maciej W. Rozycki <[email protected]>
Reported-by: Anders Blomdell <[email protected]>
Fixes: dc56ecb81a0a ("serial: 8250: Support disabling mdelay-filled probes of 16550A variants")
Cc: [email protected] # v5.6+
---
drivers/tty/serial/8250/8250_port.c | 3 ++-
include/linux/serial_core.h | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)

linux-serial-8250-full-probe.diff
Index: linux-macro/drivers/tty/serial/8250/8250_port.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/8250/8250_port.c
+++ linux-macro/drivers/tty/serial/8250/8250_port.c
@@ -1021,7 +1021,8 @@ static void autoconfig_16550a(struct uar
up->port.type = PORT_16550A;
up->capabilities |= UART_CAP_FIFO;

- if (!IS_ENABLED(CONFIG_SERIAL_8250_16550A_VARIANTS))
+ if (!IS_ENABLED(CONFIG_SERIAL_8250_16550A_VARIANTS) &&
+ !(up->port.flags & UPF_FULL_PROBE))
return;

/*
Index: linux-macro/include/linux/serial_core.h
===================================================================
--- linux-macro.orig/include/linux/serial_core.h
+++ linux-macro/include/linux/serial_core.h
@@ -414,7 +414,7 @@ struct uart_icount {
__u32 buf_overrun;
};

-typedef unsigned int __bitwise upf_t;
+typedef __u64 __bitwise upf_t;
typedef unsigned int __bitwise upstat_t;

struct uart_port {
@@ -522,6 +522,7 @@ struct uart_port {
#define UPF_FIXED_PORT ((__force upf_t) (1 << 29))
#define UPF_DEAD ((__force upf_t) (1 << 30))
#define UPF_IOREMAP ((__force upf_t) (1 << 31))
+#define UPF_FULL_PROBE ((__force upf_t) (1ULL << 32))

#define __UPF_CHANGE_MASK 0x17fff
#define UPF_CHANGE_MASK ((__force upf_t) __UPF_CHANGE_MASK)


2022-09-19 05:11:08

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing

On 17. 09. 22, 12:07, Maciej W. Rozycki wrote:
> --- linux-macro.orig/include/linux/serial_core.h
> +++ linux-macro/include/linux/serial_core.h
> @@ -414,7 +414,7 @@ struct uart_icount {
> __u32 buf_overrun;
> };
>
> -typedef unsigned int __bitwise upf_t;
> +typedef __u64 __bitwise upf_t;

Why __u64 and not u64?

> typedef unsigned int __bitwise upstat_t;
>
> struct uart_port {
> @@ -522,6 +522,7 @@ struct uart_port {
> #define UPF_FIXED_PORT ((__force upf_t) (1 << 29))
> #define UPF_DEAD ((__force upf_t) (1 << 30))
> #define UPF_IOREMAP ((__force upf_t) (1 << 31))
> +#define UPF_FULL_PROBE ((__force upf_t) (1ULL << 32))

This looks like a perfect time to switch them all to BIT_ULL().

thanks,
--
js
suse labs

2022-09-19 08:46:30

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing

On Mon, 19 Sep 2022, Jiri Slaby wrote:

> > --- linux-macro.orig/include/linux/serial_core.h
> > +++ linux-macro/include/linux/serial_core.h
> > @@ -414,7 +414,7 @@ struct uart_icount {
> > __u32 buf_overrun;
> > };
> > -typedef unsigned int __bitwise upf_t;
> > +typedef __u64 __bitwise upf_t;
>
> Why __u64 and not u64?

For consistency as there's `__u32' used elsewhere in this file. It's not
clear to me what our rules WRT the use of `s*'/`u*' vs `__s*'/`__u*' are.
I don't think we have it mentioned under Documentation/. Please clarify
if you know and I can update the change accordingly.

> > @@ -522,6 +522,7 @@ struct uart_port {
> > #define UPF_FIXED_PORT ((__force upf_t) (1 << 29))
> > #define UPF_DEAD ((__force upf_t) (1 << 30))
> > #define UPF_IOREMAP ((__force upf_t) (1 << 31))
> > +#define UPF_FULL_PROBE ((__force upf_t) (1ULL << 32))
>
> This looks like a perfect time to switch them all to BIT_ULL().

Good point, I keep forgetting about that macro. I'll keep this part
unchanged for the purpose of backporting and add 3/3 to switch all the
macros over.

Maciej

2022-09-19 11:10:39

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing

On 19. 09. 22, 10:18, Maciej W. Rozycki wrote:
> On Mon, 19 Sep 2022, Jiri Slaby wrote:
>
>>> --- linux-macro.orig/include/linux/serial_core.h
>>> +++ linux-macro/include/linux/serial_core.h
>>> @@ -414,7 +414,7 @@ struct uart_icount {
>>> __u32 buf_overrun;
>>> };
>>> -typedef unsigned int __bitwise upf_t;
>>> +typedef __u64 __bitwise upf_t;
>>
>> Why __u64 and not u64?
>
> For consistency as there's `__u32' used elsewhere in this file. It's not
> clear to me what our rules WRT the use of `s*'/`u*' vs `__s*'/`__u*' are.
> I don't think we have it mentioned under Documentation/. Please clarify
> if you know and I can update the change accordingly.

The rule is, AFAICT, use __u*/__s* in user (uapi) headers. Everywhere
else, use u*/s*.

>>> @@ -522,6 +522,7 @@ struct uart_port {
>>> #define UPF_FIXED_PORT ((__force upf_t) (1 << 29))
>>> #define UPF_DEAD ((__force upf_t) (1 << 30))
>>> #define UPF_IOREMAP ((__force upf_t) (1 << 31))
>>> +#define UPF_FULL_PROBE ((__force upf_t) (1ULL << 32))
>>
>> This looks like a perfect time to switch them all to BIT_ULL().
>
> Good point, I keep forgetting about that macro. I'll keep this part
> unchanged for the purpose of backporting and add 3/3 to switch all the
> macros over.

OK.

thanks,
--
js
suse labs

2022-09-21 00:12:45

by Maciej W. Rozycki

[permalink] [raw]
Subject: Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing

On Mon, 19 Sep 2022, Jiri Slaby wrote:

> > > Why __u64 and not u64?
> >
> > For consistency as there's `__u32' used elsewhere in this file. It's not
> > clear to me what our rules WRT the use of `s*'/`u*' vs `__s*'/`__u*' are.
> > I don't think we have it mentioned under Documentation/. Please clarify
> > if you know and I can update the change accordingly.
>
> The rule is, AFAICT, use __u*/__s* in user (uapi) headers. Everywhere else,
> use u*/s*.

Fair enough, that's consistent with ISO C's designation of identifiers
whose names start with an underscore as reserved (for system use, etc.).

Maciej