linux-kernel - Re: [PATCH v8 tty-next 2/4] serial: 8250_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]

Message-ID: <Y5T2ymgsCQhggtvz@smile.fi.intel.com>
Date:   Sat, 10 Dec 2022 23:14:50 +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, 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 Sun, Dec 11, 2022 at 07:17:28AM +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.

...

> +static int pci1xxxx_get_max_port(int subsys_dev)
> +{
> +	static int max_port[] = {
> +		1,/* PCI12000 PCI11010 PCI11101 PCI11400 */

I would put the commas in between in the comment, or is it the name of a single
product?

> +		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 you move this outside of the function you may use static_assert(), see below
why.

> +	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
> +		return max_port[subsys_dev];

Too many redundant 'else'.

	if (subsys_dev <= PCI_SUBDEVICE_ID_EFAR_PCI1XXXX_1p3)
		return max_port[subsys_dev];

(however better to compare with your size of the array)

	if (subsys_dev <= ARRAY_SIZE(max_port))
		return max_port[subsys_dev];

(in this case you can make sure it is the same as the above using
 static_assert(), so it won't compile otherwise)

	if (subsys_dev != PCI_SUBDEVICE_ID_EFAR_PCI11414)
		return max_port[0];

	return 4;

> +}

...

> +static int pci1xxxx_logical_to_physical_port_translate(int subsys_dev, int port)
> +{
> +	static int logical_to_physical_port_idx[][MAX_PORTS] = {
> +		{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
> +		return logical_to_physical_port_idx[subsys_dev][port];

Similar comments as per above function.

> +}

...

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

You issued a new version of the series without settling on this.
As you said there will be no hardware that uses IO ports, why do
you need pci_iomap()? I guess what you may use pci_ioremap_bar().

> +	if (!priv->membase)
> +		return -ENOMEM;

...

> +	priv->pdev = pdev;

> +	subsys_dev = priv->pdev->subsystem_device;

Why use priv?

> +	priv->nr = nr_ports;
> +	pci_set_master(pdev);
> +	max_vec_reqd = pci1xxxx_get_max_port(subsys_dev);

The above needs a bit of reshuffling and perhaps a blank line or lines.
Make it ordered and grouped more logically.

...

> +	num_vectors = pci_alloc_irq_vectors(pdev, 1, max_vec_reqd,
> +					    PCI_IRQ_ALL_TYPES);

I would leave this on a single line (you have such a long lines already in your
code).

> +	if (num_vectors < 0)
> +		return num_vectors;

...

> +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) },

Can be simplified a bit by PCI_VDEVICE().

> +	{}
> +};


-- 
With Best Regards,
Andy Shevchenko