| 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
| ||
|
Message-ID: <CAHp75VeDsVt0GQYUFxLM+obfmqXBPa3hM3YMsFbc26uzWZG-SQ@mail.gmail.com>
Date: Sun, 30 Nov 2025 21:22:20 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Crescent Hsieh <crescentcy.hsieh@...a.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org,
ilpo.jarvinen@...ux.intel.com, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH v1 09/31] serial: 8250: split 8250_mxpcie from 8250_pci
On Sun, Nov 30, 2025 at 12:44 PM Crescent Hsieh
<crescentcy.hsieh@...a.com> wrote:
Thanks for the patch, my comments below.
> To avoid polluting 8250_pci, as suggested by Andy Shevchenko,
Suggested-by: tag?
> this patch
> separates 8250_mxpcie into its own module.
Imperative voice.
> - https://lore.kernel.org/all/ZmQovC6TbDpTb3c8@surfacebook.localdomain/
This can be made
Link: ... [1]
and referred to in the text as [1].
...
> + * 8250_mxpcie.c - Moxa PCIe multiport serial device driver
No filenames in the file, it makes it desynchronised in case of rename
as people often to forgot to update this.
...
> +#include <linux/module.h>
> +#include <linux/pci.h>
This needs more headers to be included
bitfield.h
bits.h
dev_printk.h
device/devres.h
ioport.h
types.h
...
> +static unsigned int mxpcie8250_get_supp_rs(unsigned short device)
> +{
> + switch (device & 0x0F00) {
GENMASK() ?
> + case 0x0000:
> + case 0x0600:
> + return MOXA_SUPP_RS232;
> + case 0x0100:
> + return MOXA_SUPP_RS232 | MOXA_SUPP_RS422 | MOXA_SUPP_RS485;
> + case 0x0300:
> + return MOXA_SUPP_RS422 | MOXA_SUPP_RS485;
> + }
> +
> + return 0;
> +}
...
> + return FIELD_GET(0x00F0, device);
Define 0x00F0 with help of GENMASK() and use it here.
...
> +static int mxpcie8250_set_interface(struct mxpcie8250 *priv,
> + unsigned int port_idx,
> + u8 mode)
> +{
> + resource_size_t iobar_addr = pci_resource_start(priv->pdev, 2);
> + resource_size_t UIR_addr = iobar_addr + MOXA_UIR_OFFSET + port_idx / 2;
> + u8 cval;
> + cval = inb(UIR_addr);
I think it's in the original code, but can we switch to
ioreadXX()/iowriteXX() at some point?
> + if (port_idx % 2) {
> + cval &= ~MOXA_ODD_RS_MASK;
> + cval |= FIELD_PREP(MOXA_ODD_RS_MASK, mode);
FIELD_MODIFY()
> + } else {
> + cval &= ~MOXA_EVEN_RS_MASK;
> + cval |= FIELD_PREP(MOXA_EVEN_RS_MASK, mode);
Ditto.
> + }
> + outb(cval, UIR_addr);
> +
> + return 0;
> +}
...
> +static int mxpcie8250_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct uart_8250_port up;
Can be just = {} and no memset() below is needed.
> + struct mxpcie8250 *priv;
> + unsigned int i, num_ports;
> + int ret;
> +
> + ret = pcim_enable_device(pdev);
> + pci_save_state(pdev);
Why here?
> +
If there is no need in the above, this blank line should be removed.
> + if (ret)
> + return ret;
> +
> + num_ports = mxpcie8250_get_nports(pdev->device);
> +
> + priv = devm_kzalloc(&pdev->dev, struct_size(priv, line, num_ports), GFP_KERNEL);
With
struct device *dev = &pdev->dev;
the probe code will look neater.
> +
stray blank line addition.
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->supp_rs = mxpcie8250_get_supp_rs(pdev->device);
> + priv->num_ports = num_ports;
> +
> + mxpcie8250_init(pdev);
> + priv->pdev = pdev;
> +
> + memset(&up, 0, sizeof(up));
> +
> + up.port.dev = &pdev->dev;
> + up.port.irq = pdev->irq;
> + up.port.uartclk = MOXA_PUART_BASE_BAUD * 16;
> + up.port.flags = UPF_SKIP_TEST | UPF_BOOT_AUTOCONF | UPF_SHARE_IRQ;
> +
> + for (i = 0; i < num_ports; i++) {
> + if (mxpcie8250_setup(pdev, priv, &up, i))
> + break;
> +
> + dev_dbg(&pdev->dev, "Setup PCI port: port %lx, irq %d, type %d\n",
> + up.port.iobase, up.port.irq, up.port.iotype);
> +
> + priv->line[i] = serial8250_register_8250_port(&up);
> +
Stray blank line addition.
> + if (priv->line[i] < 0) {
> + dev_err(&pdev->dev,
> + "Couldn't register serial port %lx, irq %d, type %d, error %d\n",
> + up.port.iobase, up.port.irq,
> + up.port.iotype, priv->line[i]);
> + break;
> + }
> + }
> + pci_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
...
> +static const struct pci_device_id mxpcie8250_pci_ids[] = {
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP102E) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP102EL) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP102N) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP104EL_A) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP104N) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP112N) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP114EL) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP114N) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP116E_A_A) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP116E_A_B) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP118EL_A) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP118E_A_I) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP132EL) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP132N) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP134EL_A) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP134N) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP138E_A) },
> + { PCI_VDEVICE(MOXA, PCI_DEVICE_ID_MOXA_CP168EL_A) },
Too many spaces (or are they tabs?) before each of }:s.
> + { }
> +};
...
> +static struct pci_driver mxpcie8250_pci_driver = {
> + .name = "8250_mxpcie",
> + .id_table = mxpcie8250_pci_ids,
> + .probe = mxpcie8250_probe,
> + .remove = mxpcie8250_remove,
> +};
> +
This blank line...
> +module_pci_driver(mxpcie8250_pci_driver);
...should be here.
> +MODULE_AUTHOR("Moxa Inc.");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Moxa PCIe Multiport Serial Device Driver");
> +MODULE_DEVICE_TABLE(pci, mxpcie8250_pci_ids);
Move this to be closer to the ID table initialisation.
> +MODULE_IMPORT_NS("SERIAL_8250_PCI");
...
Shouldn't MOXA devices be blacklisted in PCI to avoid enumeration by PCI class?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists