Skip to contentSkip to conversation

serial: 8250_exar: Bug fix for determination of number of ports for Commtech PCIe cards

PATCH In review Valmer Huhn 2 participants 3 messages Aug 12, 2020 – Aug 13, 2020 TTY layer and serial drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 serial: 8250_exar: Bug fix for determination of number of ports for Commtech PCIe cards

serial: 8250_exar: Bug fix for determination of number of ports for Commtech PCIe cards

The following line is used to determine the number of ports for each exar board in a/drivers/tty/serial/8250/8250_exar.c:589

nr_ports = board->num_ports ? board->num_ports : pcidev->device & 0x0f;

If the number of ports a card has is not explicitly specified, it defaults to the rightmost 4 bits of the PCI device ID. This is prone to error since not all PCI device IDs contain a number which corresponds to the number of ports that card provides.

This particular case involves COMMTECH_4224PCIE and COMMTECH_4228PCIE cards with device ID 0x0020 and 0x0021. Currently the multiport cards receive 0 and 1 port instead of 4 and 8 ports respectively.

To fix this, each Commtech Fastcom PCIe card is given a struct where the number of ports is explicitly specified. This ensures 'board->num_ports' is used instead of the default 'pcidev->device & 0x0f'.

---
Thank you,
Valmer Huhn

drivers/tty/serial/8250/8250_exar.c +21 -3
index ddb6aeb76dc5..793906afb0f6 100644
@@ -734,6 +734,24 @@ static const struct exar8250_board pbn_exar_XR17V35x = {
 	.exit		= pci_xr17v35x_exit,
 };
 
+static const struct exar8250_board pbn_fastcom_XR17V352 = {
+	.num_ports	= 2,
+	.setup		= pci_xr17v35x_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+static const struct exar8250_board pbn_fastcom_XR17V354 = {
+	.num_ports	= 4,
+	.setup		= pci_xr17v35x_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
+static const struct exar8250_board pbn_fastcom_XR17V358 = {
+	.num_ports	= 8,
+	.setup		= pci_xr17v35x_setup,
+	.exit		= pci_xr17v35x_exit,
+};
+
 static const struct exar8250_board pbn_exar_XR17V4358 = {
 	.num_ports	= 12,
 	.setup		= pci_xr17v35x_setup,
@@ -801,9 +819,9 @@ static const struct pci_device_id exar_pci_tbl[] = {
 	EXAR_DEVICE(EXAR, XR17V358, pbn_exar_XR17V35x),
 	EXAR_DEVICE(EXAR, XR17V4358, pbn_exar_XR17V4358),
 	EXAR_DEVICE(EXAR, XR17V8358, pbn_exar_XR17V8358),
-	EXAR_DEVICE(COMMTECH, 4222PCIE, pbn_exar_XR17V35x),
-	EXAR_DEVICE(COMMTECH, 4224PCIE, pbn_exar_XR17V35x),
-	EXAR_DEVICE(COMMTECH, 4228PCIE, pbn_exar_XR17V35x),
+	EXAR_DEVICE(COMMTECH, 4222PCIE, pbn_fastcom_XR17V352),
+	EXAR_DEVICE(COMMTECH, 4224PCIE, pbn_fastcom_XR17V354),
+	EXAR_DEVICE(COMMTECH, 4228PCIE, pbn_fastcom_XR17V358),
 
 	EXAR_DEVICE(COMMTECH, 4222PCI335, pbn_fastcom335_2),
 	EXAR_DEVICE(COMMTECH, 4224PCI335, pbn_fastcom335_4),
#1 lore raw

On Wed, Aug 12, 2020 at 05:49:37PM -0400, Valmer Huhn wrote:

Thanks for the report. It needs a slight corrections I tell about below.

Quoted 2 lines

serial: 8250_exar: Bug fix for determination of number of ports for Commtech PCIe cards

This should not be present in the body.
And in the subject it would be better to reduce the text to something like

serial: 8250_exar: Fix number of ports for Commtech PCIe cards

Quoted 2 lines

The following line is used to determine the number of ports for each exar board in a/drivers/tty/serial/8250/8250_exar.c:589

There is no a/ folder and we don't need the full path anyway, just something like '8250_exar.c line 589' is enough.

Also refer to Exar with capitalized name.

Quoted 9 lines

nr_ports = board->num_ports ? board->num_ports : pcidev->device & 0x0f;

If the number of ports a card has is not explicitly specified, it defaults to the rightmost 4 bits of the PCI device ID. This is prone to error since not all PCI device IDs contain a number which corresponds to the number of ports that card provides.

This particular case involves COMMTECH_4224PCIE and COMMTECH_4228PCIE cards with device ID 0x0020 and 0x0021. Currently the multiport cards

'...with device IDs 0x0020, 0x0021 and 0x0022.'

Quoted 1 line

receive 0 and 1 port instead of 4 and 8 ports respectively.

and update this accordingly.

Quoted 3 lines

To fix this, each Commtech Fastcom PCIe card is given a struct where the number of ports is explicitly specified. This ensures 'board->num_ports' is used instead of the default 'pcidev->device & 0x0f'.

Please, add a Fixes tag.

Fixes d0aeaa83f0b0 ("serial: exar: split out the exar code from 8250_pci")
Quoted 2 lines

...

drivers/tty/serial/8250/8250_exar.c static const struct pci_device_id exar_pci_tbl[] = { · line 824

Quoted 6 lines
drivers/tty/serial/8250/8250_exar.c +3 -3
-	EXAR_DEVICE(COMMTECH, 4222PCIE, pbn_exar_XR17V35x),
-	EXAR_DEVICE(COMMTECH, 4224PCIE, pbn_exar_XR17V35x),
-	EXAR_DEVICE(COMMTECH, 4228PCIE, pbn_exar_XR17V35x),
+	EXAR_DEVICE(COMMTECH, 4222PCIE, pbn_fastcom_XR17V352),
+	EXAR_DEVICE(COMMTECH, 4224PCIE, pbn_fastcom_XR17V354),
+	EXAR_DEVICE(COMMTECH, 4228PCIE, pbn_fastcom_XR17V358),

For sake of the consistency I would rather see them as

pbn_fastcom35x_2/4/8.

Quoted 2 lines
drivers/tty/serial/8250/8250_exar.c +0 -0
 	EXAR_DEVICE(COMMTECH, 4222PCI335, pbn_fastcom335_2),
 	EXAR_DEVICE(COMMTECH, 4224PCI335, pbn_fastcom335_4),
Signature · 2 lines
With Best Regards,
Andy Shevchenko
#2 lore raw
Quoted 3 lines · Andy Shevchenko

On Wed, Aug 12, 2020 at 05:49:37PM -0400, Valmer Huhn wrote:

Thanks for the report. It needs a slight corrections I tell about below.

Thank you very much for your feedback.

Quoted 7 lines
Quoted 2 lines

serial: 8250_exar: Bug fix for determination of number of ports for Commtech PCIe cards

This should not be present in the body.
And in the subject it would be better to reduce the text to something like

serial: 8250_exar: Fix number of ports for Commtech PCIe cards

I will resubmit the patch with this subject line and include the changes you suggested.

Thank you,
Valmer Huhn