linux-kernel - Re: [PATCH v5 tty-next 1/4] 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:   Thu, 17 Nov 2022 10:25:13 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Kumaravel Thiagarajan <kumaravel.thiagarajan@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
        gregkh@...uxfoundation.org, jirislaby@...nel.org,
        ilpo.jarvinen@...ux.intel.com, macro@...am.me.uk,
        jay.dolan@...esio.com, cang1@...e.co.uk,
        u.kleine-koenig@...gutronix.de, wander@...hat.com,
        etremblay@...tech-controls.com, jk@...abs.org,
        biju.das.jz@...renesas.com, geert+renesas@...der.be,
        phil.edworthy@...esas.com, lukas@...ner.de,
        UNGLinuxDriver@...rochip.com, colin.i.king@...il.com,
        Tharun Kumar P <tharunkumar.pasumarthi@...rochip.com>
Subject: Re: [PATCH v5 tty-next 1/4] 8250: microchip: pci1xxxx: Add driver
 for quad-uart support.

On Thu, Nov 17, 2022 at 10:31:23AM +0530, 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.

Getting better!

...

> +struct pci1xxxx_8250 {
> +	struct pci_dev *dev;

Call it pdev to distinguish with regular struct device.

> +	unsigned int nr;
> +	void __iomem *membase;
> +	int line[];
> +};

...

> +static int pci1xxxx_get_num_ports(struct pci_dev *dev)
> +{
> +	switch (dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:

> +	default:

You can even start with a default, so it will be more visible.
But the way it's now is also okay.

> +		return 1;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> +		return 2;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> +		return 3;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> +		return 4;
> +	}
> +}

...

> +	quot = (NSEC_PER_SEC / (baud * UART_BIT_SAMPLE_CNT));

Too many parentheses.

> +	*frac = (((NSEC_PER_SEC - (quot * baud * UART_BIT_SAMPLE_CNT)) /

Ditto.

> +		  UART_BIT_SAMPLE_CNT) * 255) / baud;

...

> +	switch (priv->dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> +		first_offset = 256;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> +		first_offset = 512;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> +		first_offset = 768;
> +		break;

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +		first_offset = 256;
> +		break;

Can't it be moved to the above list?

> +	default:
> +		first_offset = 0;
> +		break;
> +	}

...

> +	switch (priv->dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> +		if (idx > 0)
> +			idx++;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> +		if (idx > 0)
> +			idx += 2;
> +		break;

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +		if (idx > 0)
> +			idx++;
> +		break;

Can it be moved to the above list?

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> +		if (idx > 1)
> +			idx++;
> +		break;

default?

> +	}

...

> +	switch (priv->dev->subsystem_device) {
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p0:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI12000:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11010:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11101:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11400:
> +	default:
> +		irq_idx = 0;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p1:
> +		irq_idx = 1;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p2:
> +		irq_idx = 2;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3:
> +		irq_idx = 3;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p01:

> +		irq_idx = idx;

This line is duplicated. I told you how to avoid duplication.
Use -1 outside of the switch-case and check that after.

> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p02:
> +		if (idx > 0)
> +			idx++;
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p03:
> +		if (idx > 0)
> +			idx += 2;
> +		irq_idx = idx;
> +		break;

> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p12:
> +		irq_idx = idx + 1;
> +		break;


> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p13:
> +		if (idx > 0)
> +			idx += 1;
> +		irq_idx = idx + 1;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_2p23:
> +		irq_idx = idx + 2;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p012:
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p013:
> +		if (idx > 1)
> +			idx++;
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p023:
> +		if (idx > 0)
> +			idx++;
> +		irq_idx = idx;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_3p123:
> +		irq_idx = idx + 1;
> +		break;
> +	case PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_4p:
> +	case PCI_SUBDEVICE_ID_EFAR_PCI11414:
> +		irq_idx = idx;
> +		break;

Try to make this entire switch-case more compact. It's possible.

> +	}

...

> +	dev = &pdev->dev;

You can do it in the definition block above, since we know that dev is always
valid at this point.

...

> +	priv->membase = pcim_iomap(pdev, 0, 0);

Never fails?

...

> +	for (i = 0; i < nr_ports; i++) {
> +		if (num_vectors == 4)
> +			pci1xxxx_irq_assign(priv, &uart, i);
> +
> +		rc = pci1xxxx_setup(priv, &uart, i);
> +		if (rc) {
> +			dev_warn(dev, "Failed to setup port %u\n", i);
> +			break;

If it's not a fatal error, why break? Don't you need to continue for the rest?
Otherwise use

	return dev_err_probe(...);

pattern.

> +		}
> +		priv->line[i] = serial8250_register_8250_port(&uart);
> +		if (priv->line[i] < 0) {
> +			dev_err(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;

Ditto.

> +		}
> +	}

-- 
With Best Regards,
Andy Shevchenko