linux-kernel - Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for quad-uart support.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Oct 2022 21:36:52 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>,
        gregkh@...uxfoundation.org, jirislaby@...nel.org,
        ilpo.jarvinen@...ux.intel.com, andy.shevchenko@...il.com,
        u.kleine-koenig@...gutronix.de, johan@...nel.org,
        wander@...hat.com, etremblay@...tech-controls.com,
        macro@...am.me.uk, geert+renesas@...der.be, jk@...abs.org,
        phil.edworthy@...esas.com, lukas@...ner.de
Cc:     linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH v2 tty-next 1/3] 8250: microchip: pci1xxxx: Add driver for
 quad-uart support.

Le 01/10/2022 à 08:15, Kumaravel Thiagarajan a écrit :
> 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.
> 
> Signed-off-by: Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>
> ---

[...]

> +static int pci1xxxx_setup_port(struct pci1xxxx_8250 *priv, struct uart_8250_port *port,
> +			       int offset)
> +{
> +	struct pci_dev *dev = priv->dev;
> +
> +	if (pci_resource_flags(dev, 0) & IORESOURCE_MEM) {
> +		if (!pcim_iomap(dev, 0, 0) && !pcim_iomap_table(dev))
> +			return -ENOMEM;
> +
> +		port->port.iotype = UPIO_MEM;
> +		port->port.iobase = 0;
> +		port->port.mapbase = pci_resource_start(dev, 0) + offset;
> +		port->port.membase = pcim_iomap_table(dev)[0] + offset;

Hi,

Is it needed to call pcim_iomap_table(dev) twice? (here and a few lines 
above in the 'if')

> +		port->port.regshift = 0;
> +	} else {
> +		port->port.iotype = UPIO_PORT;
> +		port->port.iobase = pci_resource_start(dev, 0) + offset;
> +		port->port.mapbase = 0;
> +		port->port.membase = NULL;
> +		port->port.regshift = 0;
> +	}
> +
> +	return 0;
> +}

[...]

> +static int pci1xxxx_serial_probe(struct pci_dev *dev,
> +				 const struct pci_device_id *ent)
> +{
> +	struct pci1xxxx_8250 *priv;
> +	struct uart_8250_port uart;
> +	unsigned int nr_ports, i;
> +	int num_vectors = 0;
> +	int rc;
> +
> +	rc = pcim_enable_device(dev);
> +	if (rc)
> +		return rc;
> +
> +	nr_ports = pci1xxxx_get_num_ports(dev);
> +
> +	priv = devm_kzalloc(&dev->dev, struct_size(priv, line, nr_ports), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->membase = pcim_iomap(dev, 0, 0);
> +	priv->dev = dev;
> +	priv->nr =  nr_ports;
> +
> +	pci_set_master(dev);
> +
> +	num_vectors  = pci_alloc_irq_vectors(dev, 1, 4, PCI_IRQ_ALL_TYPES);
> +	if (num_vectors < 0)
> +		return num_vectors;
> +
> +	memset(&uart, 0, sizeof(uart));
> +	uart.port.flags = UPF_SHARE_IRQ | UPF_FIXED_TYPE | UPF_FIXED_PORT;
> +	uart.port.uartclk = 62500000;
> +	uart.port.dev = &dev->dev;
> +
> +	if (num_vectors == 4)
> +		writeb(UART_PCI_CTRL_SET_MULTIPLE_MSI, (priv->membase + UART_PCI_CTRL_REG));
> +	else
> +		uart.port.irq = pci_irq_vector(dev, 0);
> +
> +	for (i = 0; i < nr_ports; i++) {
> +		if (num_vectors == 4)
> +			pci1xxxx_irq_assign(priv, &uart, i);
> +		priv->line[i] = -ENOSPC;
> +		rc = pci1xxxx_setup(priv, &uart, i);
> +		if (rc) {
> +			dev_err(&dev->dev, "Failed to setup port %u\n", i);
> +			break;
> +		}
> +		priv->line[i] = serial8250_register_8250_port(&uart);

In case of error, this should be undone in an error handling path in the 
probe, as done in the remove() function below.

If we break, we still continue and return success. But the last 
priv->line[i] are still 0. Is it an issue when pci1xxxx_serial_remove() 
is called?

> +
> +		if (priv->line[i] < 0) {
> +			dev_err(&dev->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]);
> +			break;
> +		}
> +	}
> +
> +	pci_set_drvdata(dev, priv);
> +
> +	return 0;
> +}
> +
> +static void pci1xxxx_serial_remove(struct pci_dev *dev)
> +{
> +	struct pci1xxxx_8250 *priv = pci_get_drvdata(dev);
> +	int i;
> +
> +	for (i = 0; i < priv->nr; i++) {
> +		if (priv->line[i] >= 0)
> +			serial8250_unregister_port(priv->line[i]);
> +	}
> +}
> +

[...]