| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Dec 2022 08:44:41 +0100
From: Jiri Slaby <jirislaby@...nel.org>
To: Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>,
linux-kernel@...r.kernel.org
Cc: linux-serial@...r.kernel.org, gregkh@...uxfoundation.org,
ilpo.jarvinen@...ux.intel.com, macro@...am.me.uk,
andriy.shevchenko@...ux.intel.com, cang1@...e.co.uk,
colin.i.king@...il.com, phil.edworthy@...esas.com,
biju.das.jz@...renesas.com, geert+renesas@...der.be,
lukas@...ner.de, u.kleine-koenig@...gutronix.de, wander@...hat.com,
etremblay@...tech-controls.com, jk@...abs.org,
UNGLinuxDriver@...rochip.com,
Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>
Subject: Re: [PATCH v8 tty-next 2/4] serial: 8250_pci1xxxx: Add driver for
quad-uart support
On 11. 12. 22, 2:47, Kumaravel Thiagarajan wrote:
> pci1xxxx is a PCIe switch with a multi-function endpoint on one of
> its downstream ports. Quad-uart is one of the functions in the
> multi-function endpoint. This driver loads for the quad-uart and
> enumerates single or multiple instances of uart based on the PCIe
> subsystem device ID.
>
> Co-developed-by: Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>
> Signed-off-by: Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>
...
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> @@ -0,0 +1,337 @@
...
> +struct pci1xxxx_8250 {
> + struct pci_dev *pdev;
> + unsigned int nr;
> + void __iomem *membase;
> + int line[];
> +};
...
> +static int pci1xxxx_get_max_port(int subsys_dev)
Both look like should be unsigned.
> +{
> + static int max_port[] = {
const unsigned
> + 1,/* PCI12000 PCI11010 PCI11101 PCI11400 */
> + 4,/* PCI4p */
> + 3,/* PCI3p012 */
> + 4,/* PCI3p013 */
> + 4,/* PCI3p023 */
> + 4,/* PCI3p123 */
> + 2,/* PCI2p01 */
> + 3,/* PCI2p02 */
> + 4,/* PCI2p03 */
> + 3,/* PCI2p12 */
> + 4,/* PCI2p13 */
> + 4,/* PCI2p23 */
> + 1,/* PCI1p0 */
> + 2,/* PCI1p1 */
> + 3,/* PCI1p2 */
> + 4,/* PCI1p3 */
> + };
> +
> + if (subsys_dev > PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3)
> + if (subsys_dev != PCI_SUBDEVICE_ID_EFAR_PCI11414)
> + return max_port[0];
> + else
> + return 4;
> + else
No need for this else. And you should use {}.
> + return max_port[subsys_dev];
> +
> +}
> +
> +static int pci1xxxx_logical_to_physical_port_translate(int subsys_dev, int port)
> +{
> + static int logical_to_physical_port_idx[][MAX_PORTS] = {
const unsigned.
> + {0, 1, 2, 3},/* PCI12000 PCI11010 PCI11101 PCI11400 PCI11414 */
> + {0, 1, 2, 3},/* PCI4p */
> + {0, 1, 2, -1},/* PCI3p012 */
> + {0, 1, 3, -1},/* PCI3p013 */
> + {0, 2, 3, -1},/* PCI3p023 */
> + {1, 2, 3, -1},/* PCI3p123 */
> + {0, 1, -1, -1},/* PCI2p01 */
> + {0, 2, -1, -1},/* PCI2p02 */
> + {0, 3, -1, -1},/* PCI2p03 */
> + {1, 2, -1, -1},/* PCI2p12 */
> + {1, 3, -1, -1},/* PCI2p13 */
> + {2, 3, -1, -1},/* PCI2p23 */
> + {0, -1, -1, -1},/* PCI1p0 */
> + {1, -1, -1, -1},/* PCI1p1 */
> + {2, -1, -1, -1},/* PCI1p2 */
> + {3, -1, -1, -1},/* PCI1p3 */
> + };
> +
> + if (subsys_dev > PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3)
> + return logical_to_physical_port_idx[0][port];
> + else
No need for this else.
> + return logical_to_physical_port_idx[subsys_dev][port];
> +}
> +
> +static int pci1xxxx_serial_probe(struct pci_dev *pdev,
> + const struct pci_device_id *id)
> +{
> + struct device *dev = &pdev->dev;
> + struct pci1xxxx_8250 *priv;
> + struct uart_8250_port uart;
> + unsigned int nr_ports, i;
> + int max_vec_reqd;
> + int num_vectors;
> + int subsys_dev;
> + int port_idx;
> + int rc;
> +
> + rc = pcim_enable_device(pdev);
> + if (rc)
> + return rc;
> +
> + nr_ports = pci1xxxx_get_num_ports(pdev);
> +
> + priv = devm_kzalloc(dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->membase = pcim_iomap(pdev, 0, 0);
> + if (!priv->membase)
> + return -ENOMEM;
> +
> + priv->pdev = pdev;
What do you need pdev in priv for? It looks superfluous after passing it
to pci1xxxx_setup().
> + subsys_dev = priv->pdev->subsystem_device;
> + priv->nr = nr_ports;
> + pci_set_master(pdev);
> + max_vec_reqd = pci1xxxx_get_max_port(subsys_dev);
> + num_vectors = pci_alloc_irq_vectors(pdev, 1, max_vec_reqd,
> + PCI_IRQ_ALL_TYPES);
> + if (num_vectors < 0)
> + return num_vectors;
> +
> + memset(&uart, 0, sizeof(uart));
> + uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_PORT;
> + uart.port.uartclk = UART_CLOCK_DEFAULT;
> + uart.port.dev = dev;
> +
> + if (num_vectors == max_vec_reqd)
> + writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI,
> + priv->membase + UART_PCI_CTRL_REG);
> +
> + for (i = 0; i < nr_ports; i++)
> + priv->line[i] = -ENODEV;
Why is this not a part of the following (same) loop?
> +
> + for (i = 0; i < nr_ports; i++) {
> + port_idx = pci1xxxx_logical_to_physical_port_translate(subsys_dev, i);
> +
> + if (num_vectors == max_vec_reqd)
> + uart.port.irq = pci_irq_vector(priv->pdev, port_idx);
> + else
> + uart.port.irq = pci_irq_vector(pdev, 0);
> +
> + rc = pci1xxxx_setup(priv, &uart, port_idx);
> + if (rc) {
> + dev_warn(dev, "Failed to setup port %u\n", i);
> + continue;
> + }
> +
> + priv->line[i] = serial8250_register_8250_port(&uart);
> + if (priv->line[i] < 0) {
> + dev_warn(dev,
> + "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> + uart.port.iobase, uart.port.irq, uart.port.iotype,
> + priv->line[i]);
> + }
> + }
> +
> + pci_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static void pci1xxxx_serial_remove(struct pci_dev *dev)
> +{
> + struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
> + int i;
unsigned as priv->nr is.
> +
> + for (i = 0; i < priv->nr; i++) {
> + if (priv->line[i] >= 0)
> + serial8250_unregister_port(priv->line[i]);
> + }
> +}
> +
> +static const struct pci_device_id pci1xxxx_pci_tbl[] = {
> + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11010) },
> + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11101) },
> + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11400) },
> + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI11414) },
> + { PCI_DEVICE(PCI_VENDOR_ID_EFAR, PCI_DEVICE_ID_EFAR_PCI12000) },
> + {}
thanks,
--
js
suse labs
Powered by blists - more mailing lists