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

Maciej W. Rozycki posted 2 patches 3 years, 6 months ago
There is a newer version of this series
[PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing
Posted by Maciej W. Rozycki 3 years, 6 months ago
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 <macro@orcam.me.uk>
Reported-by: Anders Blomdell <anders.blomdell@control.lth.se>
Fixes: dc56ecb81a0a ("serial: 8250: Support disabling mdelay-filled probes of 16550A variants")
Cc: stable@vger.kernel.org # 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)
Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing
Posted by Jiri Slaby 3 years, 6 months ago
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
Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing
Posted by Maciej W. Rozycki 3 years, 6 months ago
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
Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing
Posted by Jiri Slaby 3 years, 6 months ago
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
Re: [PATCH 1/2] serial: 8250: Let drivers request full 16550A feature probing
Posted by Maciej W. Rozycki 3 years, 6 months ago
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