diff mbox series

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

Message ID alpine.DEB.2.21.2202121706060.34636@angie.orcam.me.uk
State Superseded
Headers show
Series serial: 8250: Correct basic issues with the PCI blacklist | expand

Commit Message

Maciej W. Rozycki Feb. 12, 2022, 5:30 p.m. UTC
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

Comments

Greg KH Feb. 25, 2022, 9:35 a.m. UTC | #1
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
Maciej W. Rozycki Feb. 26, 2022, 10:32 a.m. UTC | #2
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
Maciej W. Rozycki Feb. 27, 2022, 11:06 p.m. UTC | #3
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
diff mbox series

Patch

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);