[PATCH v2 0/2] serial: 8250: Correct basic issues with the PCI blacklist

[PATCH v2 0/2] serial: 8250: Correct basic issues with the PCI blacklist

[Maciej W. Rozycki]

Hi,

 This v2 of the patch series adds missing filler member initialisers for 
blacklist entries using the PCI_DEVICE rather than PCI_VDEVICE macro that 
cause "initializer element is not computable at load time" compilation 
errors in some configurations, which have escaped my verification.

 An upside is I have noticed PARPORT_SERIAL (or indeed PARPORT_PC) is not 
selectable with numerous PCI platforms like RISC-V meaning that PC-style 
PCI parallel port hardware cannot be used with them even though there's no 
reason for that.  The only PCI platforms that actually cannot make use of 
such hardware are those newer PCIe systems that have no support for I/O 
cycles in the host bridge with the only actual specimen known to me being 
the POWER9 PHB4 device, so we ought to enable PARPORT_PC/PARPORT_SERIAL 
support for PCI systems in the general case.  I'll post a fix separately.

 The original cover letter continues.

 In the course of investigating whether support code for OxSemi PCIe UARTs 
could be factored out from the common 8250 PCI UART driver, which has been 
previously requested by Andy (cc-ed), I have noticed that the Kconfig help 
text for several device-specific UART drivers previously factored out is 
incorrect in that it claims that those dedicated drivers are required for 
extra features of the respective devices, while actually the blacklist 
entries within the common driver make them require those dedicated drivers 
even for standard features, as the common driver now refuses to handle 
them.

 Also it may be unclear for the user from a specific PCI device ID of an 
affected PCI UART device which dedicated driver has to be configured in to 
handle it, so make the blacklist entries include that information to be 
printed if a device is encountered that cannot be handled because its 
dedicated driver has been excluded from configuration while the common 
driver refuses to handle it.

 See the respective change descriptions for further details.  Please 
apply.

  Maciej

[PATCH v2 1/2] serial: 8250: Correct Kconfig help text for blacklisted PCI devices

[Maciej W. Rozycki]

Correct the Kconfig help text for SERIAL_8250_LPSS, SERIAL_8250_MID and 
SERIAL_8250_PERICOM configuration options for dedicated PCI UART drivers 
that have been blacklisted in the generic PCI 8250 UART driver and as 
from commit a13e19cf3dc10 ("serial: 8250_lpss: split LPSS driver to 
separate module"), commit d9eda9bab2372 ("serial: 8250_pci: Intel MID 
UART support to its own driver"), and commit fcfd3c09f4078 ("serial: 
8250_pci: Split out Pericom driver") respectively are not handled by 
said driver anymore (rather than for extra features only, as the current 
text indicates), and therefore require the respective dedicated drivers 
to work at all.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
No changes from v1.
---
 drivers/tty/serial/8250/Kconfig |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

linux-serial-8250-pci-blacklist-help.diff
Index: linux-macro/drivers/tty/serial/8250/Kconfig
===================================================================
--- linux-macro.orig/drivers/tty/serial/8250/Kconfig
+++ linux-macro/drivers/tty/serial/8250/Kconfig
@@ -479,11 +479,12 @@ config SERIAL_8250_LPSS
 	select DW_DMAC_PCI if (SERIAL_8250_DMA && X86_INTEL_LPSS)
 	select RATIONAL
 	help
-	  Selecting this option will enable handling of the extra features
-	  present on the UART found on various Intel platforms such as:
+	  Selecting this option will enable handling of the UART found on
+	  various Intel platforms such as:
 	    - Intel Baytrail SoC
 	    - Intel Braswell SoC
 	    - Intel Quark X1000 SoC
+	  that are not covered by the more generic SERIAL_8250_PCI option.
 
 config SERIAL_8250_MID
 	tristate "Support for serial ports on Intel MID platforms"
@@ -494,17 +495,18 @@ config SERIAL_8250_MID
 	select HSU_DMA_PCI if (HSU_DMA && X86_INTEL_MID)
 	select RATIONAL
 	help
-	  Selecting this option will enable handling of the extra features
-	  present on the UART found on Intel Medfield SOC and various other
-	  Intel platforms.
+	  Selecting this option will enable handling of the UART found on
+	  Intel Medfield SOC and various other Intel platforms that is not
+	  covered by the more generic SERIAL_8250_PCI option.
 
 config SERIAL_8250_PERICOM
 	tristate "Support for Pericom and Acces I/O serial ports"
 	default SERIAL_8250
 	depends on SERIAL_8250 && PCI
 	help
-	  Selecting this option will enable handling of the extra features
-	  present on the Pericom and Acces I/O UARTs.
+	  Selecting this option will enable handling of the Pericom and Acces
+	  I/O UARTs that are not covered by the more generic SERIAL_8250_PCI
+	  option.
 
 config SERIAL_8250_PXA
 	tristate "PXA serial port support"

[PATCH v2 2/2] serial: 8250: Report which option to enable for blacklisted PCI devices

[Maciej W. Rozycki]

Provide information in the kernel log as to what configuration option to 
enable for PCI UART devices that have been blacklisted in the generic 
PCI 8250 UART driver and which have a dedicated driver available to 
handle that has been disabled.  The rationale is there is no easy way 
for the user to map a specific PCI vendor:device pair to an individual 
dedicated driver while the generic driver has this information readily 
available and it will likely be confusing that the generic driver does 
not register such a port.

A message is then printed like:

serial 0000:04:00.3: ignoring port, enable SERIAL_8250_PERICOM to handle

when an affected device is encountered and the generic driver rejects it.

Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
---
Hi,

 Verified in a simulated environment as obviously I don't have a Pericom 
device.

  Maciej

Changes from v1:

- Add missing filler struct member initialisers for PCI_DEVICE entries.
---
 drivers/tty/serial/8250/8250_pci.c |   67 ++++++++++++++++++++++---------------
 1 file changed, 40 insertions(+), 27 deletions(-)

linux-serial-8250-pci-blacklist-config.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
@@ -3518,6 +3518,12 @@ static struct pciserial_board pci_boards
 	},
 };
 
+#define REPORT_CONFIG(option) \
+	(IS_ENABLED(CONFIG_##option) ? 0 : (kernel_ulong_t)&#option)
+#define REPORT_8250_CONFIG(option) \
+	(IS_ENABLED(CONFIG_SERIAL_8250_##option) ? \
+	 0 : (kernel_ulong_t)&"SERIAL_8250_"#option)
+
 static const struct pci_device_id blacklist[] = {
 	/* softmodems */
 	{ PCI_VDEVICE(AL, 0x5457), }, /* ALi Corporation M5457 AC'97 Modem */
@@ -3525,40 +3531,43 @@ static const struct pci_device_id blackl
 	{ PCI_DEVICE(0x1543, 0x3052), }, /* Si3052-based modem, default IDs */
 
 	/* multi-io cards handled by parport_serial */
-	{ PCI_DEVICE(0x4348, 0x7053), }, /* WCH CH353 2S1P */
-	{ PCI_DEVICE(0x4348, 0x5053), }, /* WCH CH353 1S1P */
-	{ PCI_DEVICE(0x1c00, 0x3250), }, /* WCH CH382 2S1P */
+	/* WCH CH353 2S1P */
+	{ PCI_DEVICE(0x4348, 0x7053), 0, 0, REPORT_CONFIG(PARPORT_SERIAL), },
+	/* WCH CH353 1S1P */
+	{ PCI_DEVICE(0x4348, 0x5053), 0, 0, REPORT_CONFIG(PARPORT_SERIAL), },
+	/* WCH CH382 2S1P */
+	{ PCI_DEVICE(0x1c00, 0x3250), 0, 0, REPORT_CONFIG(PARPORT_SERIAL), },
 
 	/* Intel platforms with MID UART */
-	{ PCI_VDEVICE(INTEL, 0x081b), },
-	{ PCI_VDEVICE(INTEL, 0x081c), },
-	{ PCI_VDEVICE(INTEL, 0x081d), },
-	{ PCI_VDEVICE(INTEL, 0x1191), },
-	{ PCI_VDEVICE(INTEL, 0x18d8), },
-	{ PCI_VDEVICE(INTEL, 0x19d8), },
+	{ PCI_VDEVICE(INTEL, 0x081b), REPORT_8250_CONFIG(MID), },
+	{ PCI_VDEVICE(INTEL, 0x081c), REPORT_8250_CONFIG(MID), },
+	{ PCI_VDEVICE(INTEL, 0x081d), REPORT_8250_CONFIG(MID), },
+	{ PCI_VDEVICE(INTEL, 0x1191), REPORT_8250_CONFIG(MID), },
+	{ PCI_VDEVICE(INTEL, 0x18d8), REPORT_8250_CONFIG(MID), },
+	{ PCI_VDEVICE(INTEL, 0x19d8), REPORT_8250_CONFIG(MID), },
 
 	/* Intel platforms with DesignWare UART */
-	{ PCI_VDEVICE(INTEL, 0x0936), },
-	{ PCI_VDEVICE(INTEL, 0x0f0a), },
-	{ PCI_VDEVICE(INTEL, 0x0f0c), },
-	{ PCI_VDEVICE(INTEL, 0x228a), },
-	{ PCI_VDEVICE(INTEL, 0x228c), },
-	{ PCI_VDEVICE(INTEL, 0x4b96), },
-	{ PCI_VDEVICE(INTEL, 0x4b97), },
-	{ PCI_VDEVICE(INTEL, 0x4b98), },
-	{ PCI_VDEVICE(INTEL, 0x4b99), },
-	{ PCI_VDEVICE(INTEL, 0x4b9a), },
-	{ PCI_VDEVICE(INTEL, 0x4b9b), },
-	{ PCI_VDEVICE(INTEL, 0x9ce3), },
-	{ PCI_VDEVICE(INTEL, 0x9ce4), },
+	{ PCI_VDEVICE(INTEL, 0x0936), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x0f0a), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x0f0c), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x228a), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x228c), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x4b96), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x4b97), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x4b98), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x4b99), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x4b9a), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x4b9b), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x9ce3), REPORT_8250_CONFIG(LPSS), },
+	{ PCI_VDEVICE(INTEL, 0x9ce4), REPORT_8250_CONFIG(LPSS), },
 
 	/* Exar devices */
-	{ PCI_VDEVICE(EXAR, PCI_ANY_ID), },
-	{ PCI_VDEVICE(COMMTECH, PCI_ANY_ID), },
+	{ PCI_VDEVICE(EXAR, PCI_ANY_ID), REPORT_8250_CONFIG(EXAR), },
+	{ PCI_VDEVICE(COMMTECH, PCI_ANY_ID), REPORT_8250_CONFIG(EXAR), },
 
 	/* Pericom devices */
-	{ PCI_VDEVICE(PERICOM, PCI_ANY_ID), },
-	{ PCI_VDEVICE(ACCESSIO, PCI_ANY_ID), },
+	{ PCI_VDEVICE(PERICOM, PCI_ANY_ID), REPORT_8250_CONFIG(PERICOM), },
+	{ PCI_VDEVICE(ACCESSIO, PCI_ANY_ID), REPORT_8250_CONFIG(PERICOM), },
 
 	/* End of the black list */
 	{ }
@@ -3840,8 +3849,12 @@ pciserial_init_one(struct pci_dev *dev,
 	board = &pci_boards[ent->driver_data];
 
 	exclude = pci_match_id(blacklist, dev);
-	if (exclude)
+	if (exclude) {
+		if (exclude->driver_data)
+			pci_warn(dev, "ignoring port, enable %s to handle\n",
+				 (const char *)exclude->driver_data);
 		return -ENODEV;
+	}
 
 	rc = pcim_enable_device(dev);
 	pci_save_state(dev);

Re: [PATCH v2 2/2] serial: 8250: Report which option to enable for blacklisted PCI devices

[Greg Kroah-Hartman]

On Sat, Feb 12, 2022 at 05:30:59PM +0000, Maciej W. Rozycki wrote:
> Provide information in the kernel log as to what configuration option to 
> enable for PCI UART devices that have been blacklisted in the generic 
> PCI 8250 UART driver and which have a dedicated driver available to 
> handle that has been disabled.  The rationale is there is no easy way 
> for the user to map a specific PCI vendor:device pair to an individual 
> dedicated driver while the generic driver has this information readily 
> available and it will likely be confusing that the generic driver does 
> not register such a port.
> 
> A message is then printed like:
> 
> serial 0000:04:00.3: ignoring port, enable SERIAL_8250_PERICOM to handle
> 
> when an affected device is encountered and the generic driver rejects it.
> 
> Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>

I've applied patch 1 of this series, but this is really an odd one.

We don't do this for any other driver subsystem, so why is it really
needed?  What is so special about this driver that distros can't
just enable all of the drivers and all is good?  What is keeping those
drivers fromb eing enabled?

thanks,

greg k-h

Re: [PATCH v2 2/2] serial: 8250: Report which option to enable for blacklisted PCI devices

[Maciej W. Rozycki]

On Fri, 25 Feb 2022, Greg Kroah-Hartman wrote:

> On Sat, Feb 12, 2022 at 05:30:59PM +0000, Maciej W. Rozycki wrote:
> > Provide information in the kernel log as to what configuration option to 
> > enable for PCI UART devices that have been blacklisted in the generic 
> > PCI 8250 UART driver and which have a dedicated driver available to 
> > handle that has been disabled.  The rationale is there is no easy way 
> > for the user to map a specific PCI vendor:device pair to an individual 
> > dedicated driver while the generic driver has this information readily 
> > available and it will likely be confusing that the generic driver does 
> > not register such a port.
> > 
> > A message is then printed like:
> > 
> > serial 0000:04:00.3: ignoring port, enable SERIAL_8250_PERICOM to handle
> > 
> > when an affected device is encountered and the generic driver rejects it.
> > 
> > Signed-off-by: Maciej W. Rozycki <macro@orcam.me.uk>
> 
> I've applied patch 1 of this series, but this is really an odd one.

 Thank you.

> We don't do this for any other driver subsystem, so why is it really
> needed?  What is so special about this driver that distros can't
> just enable all of the drivers and all is good?  What is keeping those
> drivers fromb eing enabled?

 My justification is we have a supposedly generic PCI 8250 UART driver, 
except it explicitly and silently refuses to handle a handful of devices 
chosen by their PCI IDs based on that they may have extra features, even 
though they are otherwise fully compatible with a generic 8250.

 For distributions it probably does not matter as long as the packager 
does not forget to enable an option, which itself might be a problem (I've 
seen distributions missing drivers randomly).  A user who configures their 
kernel on their own may simply not be aware that for one card enabling 
SERIAL_8250_PCI will do while for another almost identical card they need 
to use SERIAL_8250_foo instead even though it's just another PCI 8250 
UART.

 Consequently someone may well waste a day trying to figure out why their 
card does not work (is it faulty perhaps, is there a configuration error 
with the hardware?).  Even if they actually realise it's a kernel config 
issue, they may still have to go through a trial-and-error experience 
trying to figure out which driver to enable.  While the generic driver 
knows perfectly well.  Then why not make people's life easier and let them 
know as well what is going on?

 I don't think we have another case like this, do we?  Hence my proposal.

 Have I made myself clear now?  What are your actual arguments against my 
reasoning?

  Maciej

Re: [PATCH v2 2/2] serial: 8250: Report which option to enable for blacklisted PCI devices

[Maciej W. Rozycki]

On Sat, 26 Feb 2022, Maciej W. Rozycki wrote:

> On Fri, 25 Feb 2022, Greg Kroah-Hartman wrote:
> 
> > We don't do this for any other driver subsystem, so why is it really
> > needed?  What is so special about this driver that distros can't
> > just enable all of the drivers and all is good?  What is keeping those
> > drivers fromb eing enabled?
> 
>  My justification is we have a supposedly generic PCI 8250 UART driver, 
> except it explicitly and silently refuses to handle a handful of devices 
> chosen by their PCI IDs based on that they may have extra features, even 
> though they are otherwise fully compatible with a generic 8250.

 Actually as it happens we do have a precedent too, as here's what I have 
just spotted on my laptop by chance when hibernating:

psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.

(with a distribution kernel, so clearly whoever packaged that has not 
enabled what might be needed).  Someone else wanted to be helpful too as 
it seems.

  Maciej

Re: [PATCH v2 2/2] serial: 8250: Report which option to enable for blacklisted PCI devices

[Maciej W. Rozycki]

On Sun, 27 Feb 2022, Maciej W. Rozycki wrote:

> > > We don't do this for any other driver subsystem, so why is it really
> > > needed?  What is so special about this driver that distros can't
> > > just enable all of the drivers and all is good?  What is keeping those
> > > drivers fromb eing enabled?
> > 
> >  My justification is we have a supposedly generic PCI 8250 UART driver, 
> > except it explicitly and silently refuses to handle a handful of devices 
> > chosen by their PCI IDs based on that they may have extra features, even 
> > though they are otherwise fully compatible with a generic 8250.
> 
>  Actually as it happens we do have a precedent too, as here's what I have 
> just spotted on my laptop by chance when hibernating:
> 
> psmouse serio1: synaptics: The touchpad can support a better bus than the too old PS/2 protocol. Make sure MOUSE_PS2_SYNAPTICS_SMBUS and RMI4_SMB are enabled to get a better touchpad experience.
> 
> (with a distribution kernel, so clearly whoever packaged that has not 
> enabled what might be needed).  Someone else wanted to be helpful too as 
> it seems.

 I have now posted v3 with these clarifications included in the change 
descriptions.  Please review.

  Maciej