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

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

[Maciej W. Rozycki]

Hi,

 A recent change has added a SERIAL_8250_16550A_VARIANTS option, which 
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.  
This has turned out problematic to a more recent update for the OxSemi 
Tornado series PCIe devices, whose new baud rate generator handling code 
actually requires switching hardware into the enhanced mode for correct 
operation, which actually requires 16550A device features to have been 
probed for.

 This small patch series fixes the issue by letting individual device 
subdrivers to request full 16550A device feature probing by means of a 
flag regardless of the SERIAL_8250_16550A_VARIANTS setting chosen.

 The changes have been verified with an OXPCIe952 device, in the native 
UART mode and a 64-bit RISC-V system as well as in the legacy UART mode 
and a 32-bit x86 system.

 Credit to Anders for reporting this issue and then working through the 
resolution.

  Maciej

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

[Maciej W. Rozycki]

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

[Jiri Slaby]

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

[Maciej W. Rozycki]

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

[Jiri Slaby]

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

[Maciej W. Rozycki]

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

[PATCH 2/2] serial: 8250: Request full 16550A feature probing for OxSemi PCIe devices

[Maciej W. Rozycki]

Oxford Semiconductor PCIe (Tornado) 950 serial port devices need to 
operate in the enhanced mode via the EFR register for the Divide-by-M 
N/8 baud rate generator prescaler to be used in their native UART mode.  
Otherwise the prescaler is fixed at 1 causing grossly incorrect baud 
rates to be programmed.

Accessing the EFR register requires 16550A features to have been probed 
for, so request this to happen regardless of SERIAL_8250_16550A_VARIANTS 
by setting UPF_FULL_PROBE in port flags.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
Reported-by: Anders Blomdell <anders.blomdell@control.lth.se>
Fixes: 366f6c955d4d ("serial: 8250: Add proper clock handling for OxSemi PCIe devices")
Cc: stable@vger.kernel.org # v5.19+
---
 drivers/tty/serial/8250/8250_pci.c |    5 +++++
 1 file changed, 5 insertions(+)

linux-serial-8250-oxsemi-efr.diff
Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
===================================================================
--- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
+++ linux-macro/drivers/tty/serial/8250/8250_pci.c
@@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
 	serial8250_do_set_mctrl(port, mctrl);
 }
 
+/*
+ * We require EFR features for clock programming, so set UPF_FULL_PROBE
+ * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS setting.
+ */
 static int pci_oxsemi_tornado_setup(struct serial_private *priv,
 				    const struct pciserial_board *board,
 				    struct uart_8250_port *up, int idx)
@@ -1239,6 +1243,7 @@ static int pci_oxsemi_tornado_setup(stru
 	struct pci_dev *dev = priv->dev;
 
 	if (pci_oxsemi_tornado_p(dev)) {
+		up->port.flags |= UPF_FULL_PROBE;
 		up->port.get_divisor = pci_oxsemi_tornado_get_divisor;
 		up->port.set_divisor = pci_oxsemi_tornado_set_divisor;
 		up->port.set_mctrl = pci_oxsemi_tornado_set_mctrl;

Re: [PATCH 2/2] serial: 8250: Request full 16550A feature probing for OxSemi PCIe devices

[Jiri Slaby]

On 17. 09. 22, 12:07, Maciej W. Rozycki wrote:
> Oxford Semiconductor PCIe (Tornado) 950 serial port devices need to
> operate in the enhanced mode via the EFR register for the Divide-by-M
> N/8 baud rate generator prescaler to be used in their native UART mode.
> Otherwise the prescaler is fixed at 1 causing grossly incorrect baud
> rates to be programmed.
> 
> Accessing the EFR register requires 16550A features to have been probed
> for, so request this to happen regardless of SERIAL_8250_16550A_VARIANTS
> by setting UPF_FULL_PROBE in port flags.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> Reported-by: Anders Blomdell <anders.blomdell@control.lth.se>
> Fixes: 366f6c955d4d ("serial: 8250: Add proper clock handling for OxSemi PCIe devices")
> Cc: stable@vger.kernel.org # v5.19+
> ---
>   drivers/tty/serial/8250/8250_pci.c |    5 +++++
>   1 file changed, 5 insertions(+)
> 
> linux-serial-8250-oxsemi-efr.diff
> Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> ===================================================================
> --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> @@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
>   	serial8250_do_set_mctrl(port, mctrl);
>   }
>   
> +/*
> + * We require EFR features for clock programming, so set UPF_FULL_PROBE
> + * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS setting.
> + */

It'd make more sense to me to move this comment right before the line 
you add below.

>   static int pci_oxsemi_tornado_setup(struct serial_private *priv,
>   				    const struct pciserial_board *board,
>   				    struct uart_8250_port *up, int idx)
> @@ -1239,6 +1243,7 @@ static int pci_oxsemi_tornado_setup(stru
>   	struct pci_dev *dev = priv->dev;
>   
>   	if (pci_oxsemi_tornado_p(dev)) {
> +		up->port.flags |= UPF_FULL_PROBE;
>   		up->port.get_divisor = pci_oxsemi_tornado_get_divisor;
>   		up->port.set_divisor = pci_oxsemi_tornado_set_divisor;
>   		up->port.set_mctrl = pci_oxsemi_tornado_set_mctrl;

thanks,
-- 
js
suse labs

Re: [PATCH 2/2] serial: 8250: Request full 16550A feature probing for OxSemi PCIe devices

[Maciej W. Rozycki]

On Mon, 19 Sep 2022, Jiri Slaby wrote:

> > linux-serial-8250-oxsemi-efr.diff
> > Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> > ===================================================================
> > --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> > +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> > @@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
> >   	serial8250_do_set_mctrl(port, mctrl);
> >   }
> >   +/*
> > + * We require EFR features for clock programming, so set UPF_FULL_PROBE
> > + * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS
> > setting.
> > + */
> 
> It'd make more sense to me to move this comment right before the line you add
> below.

 I favour the style where what a function does is documented above it, but 
I won't insist on it if having a comment within is what we prefer here.

  Maciej

Re: [PATCH 2/2] serial: 8250: Request full 16550A feature probing for OxSemi PCIe devices

[Maciej W. Rozycki]

On Mon, 19 Sep 2022, Maciej W. Rozycki wrote:

> > > linux-serial-8250-oxsemi-efr.diff
> > > Index: linux-macro/drivers/tty/serial/8250/8250_pci.c
> > > ===================================================================
> > > --- linux-macro.orig/drivers/tty/serial/8250/8250_pci.c
> > > +++ linux-macro/drivers/tty/serial/8250/8250_pci.c
> > > @@ -1232,6 +1232,10 @@ static void pci_oxsemi_tornado_set_mctrl
> > >   	serial8250_do_set_mctrl(port, mctrl);
> > >   }
> > >   +/*
> > > + * We require EFR features for clock programming, so set UPF_FULL_PROBE
> > > + * for full probing regardless of CONFIG_SERIAL_8250_16550A_VARIANTS
> > > setting.
> > > + */
> > 
> > It'd make more sense to me to move this comment right before the line you add
> > below.
> 
>  I favour the style where what a function does is documented above it, but 
> I won't insist on it if having a comment within is what we prefer here.

 Having looked at it again I changed my mind and decided it'll be more 
consistent with the rest of the code if this comment remains above the 
function after all.

 My rationale is it is the only function for OxSemi Tornado devices still 
without an introductory comment, the other functions have their internals 
documented solely within their leading comments, and last but not least it 
is obvious what the comment refers to, especially as the function is so 
small (as to fit even in an 80x24 character glass TTY device).

 I have posted v2 with your other suggestions applied.  Thank you for your 
review.

  Maciej

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

[Josh Triplett]

On September 17, 2022 11:07:18 AM GMT+01:00, "Maciej W. Rozycki" <macro@orcam.me.uk> wrote:
>Hi,
>
> A recent change has added a SERIAL_8250_16550A_VARIANTS option, which 
>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.  
>This has turned out problematic to a more recent update for the OxSemi 
>Tornado series PCIe devices, whose new baud rate generator handling code 
>actually requires switching hardware into the enhanced mode for correct 
>operation, which actually requires 16550A device features to have been 
>probed for.
>
> This small patch series fixes the issue by letting individual device 
>subdrivers to request full 16550A device feature probing by means of a 
>flag regardless of the SERIAL_8250_16550A_VARIANTS setting chosen.
>
> The changes have been verified with an OXPCIe952 device, in the native 
>UART mode and a 64-bit RISC-V system as well as in the legacy UART mode 
>and a 32-bit x86 system.

Seems reasonable to me, as long as the flag is only set by drivers that know they've found their hardware.

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

[Maciej W. Rozycki]

On Sat, 17 Sep 2022, Josh Triplett wrote:

> > This small patch series fixes the issue by letting individual device 
> >subdrivers to request full 16550A device feature probing by means of a 
> >flag regardless of the SERIAL_8250_16550A_VARIANTS setting chosen.
> >
> > The changes have been verified with an OXPCIe952 device, in the native 
> >UART mode and a 64-bit RISC-V system as well as in the legacy UART mode 
> >and a 32-bit x86 system.
> 
> Seems reasonable to me, as long as the flag is only set by drivers that 
> know they've found their hardware.

 That has been my intent or otherwise the change would make no sense as 
far as I am concerned.

 In principle for most if not all PCI/e devices we could suppress UART 
probing altogether and still support device's features as we could infer 
the features from the vendor:device ID pair via a table of per-device 
flags.  This might even have worked if we started making one right from 
the beginning as individual devices were added to our 8250/PCI driver.

 Though I can imagine that for some devices no documentation was available 
to the contributor and it could have been hard to determine whether a 
feature actually discovered is really guaranteed for a given vendor:device 
ID or whether there are additional constraints, such as a device revision.  

 I imagine especially early PCI serial port devices may have used discrete 
UART chips behind a piece of PCI glue (just as we now see numerous PCIe 
devices with the actual device placed behind a PCIe-to-PCI bridge onboard) 
and then the set of features could have depended on the specific UART 
chips chosen which may have changed in the course of the life of product.

 At this point however I suspect it would be hard to (re)construct such a 
table and in any case it could have been a maintenance burden, so I guess 
we need to live with what we have.

 Thank you for your input.

  Maciej