| 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: <20190315101059.25096-1-saumah@gmail.com>
Date: Fri, 15 Mar 2019 18:10:59 +0800
From: Morris Ku <saumah@...il.com>
To: lkml@...ux.net
Cc: gregkh@...uxfoundation.org, morris_ku@...ix.com,
linux-kernel@...r.kernel.org, arnd@...db.de,
Morris Ku <saumah@...il.com>
Subject: [PATCH] Respond:add support for SUNIX Multi-I/O board
Hi Enrico,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <saumah@...il.com>
---
+From 5b1c4c8f7d91661a27c88b980c42b768e4cb7606 Mon Sep 17 00:00:00 2001
+From: Morris Ku <saumah@...il.com>
+Date: Tue, 26 Feb 2019 17:11:48 +0800
+Subject: [PATCH 6/6] add support for SUNIX Multi-I/O board
+
+
+<snip>
+
+> diff --git a/char/snx/snx_main.c b/char/snx/snx_main.c
+
+if it's a serial card, shouldn't it go to drivers/tty/serial/snx/ ?
+
i will move to drivers/mfd (multi function devices)
+
+
+<snip>
+
+> +char
+[SNX_SER_PORT_MAX_UART][10] = {
+> + {"UNKNOWN"},
+> + {"SUN1889"},
+> + {"SUN1699"},
+> + {"SUNMATX"},
+> + {"SUN1999"}
+> +};
+> +
+> +char snx_par_ic_table[SNX_PAR_PORT_MAX_UART][10] = {
+> + {"UNKNOWN"},
+> + {"SUN1888"},
+> + {"SUN1689"},
+> + {"SUNMATX"},
+> + {"SUN1999"}
+> +};
+> +
+> +char snx_port_remap[2][10] = {
+> + {"NON-REMAP"},
+> + {"REMAP"}
+> +};
+
+can't these be const static ?
+
i will fix it.
+
+can we have a bit more consistent formatting (eg. all those comments
+w/ same indention) ?
+
i prefer to keep it in current format.
+
+> + SUN1999_BOARD_5008A,
+> +
+
+<snip>
+
+> +static struct pci_device_id sunix_pci_board_id[] = {
+> +// golden-serial
+
+can't this be const ?
+
i will fix it.
+
+(maybe even __initconst)
+I'd really prefer separate port types - that go into separate
+subsystems, in separate modules (those can share code in a common
+module, if desired)
+
+Oh, is there really a hard restriction on the max number of boards ?
+
+driver support maximum 4 boards can be installed incombination
+(up to 32 serial port and 2 parallel port)
+
+And do we really need a global list of them ? (instead of just having
+all per-board / per-port data in a per-board / per-port driver instance)
+
+
i prefer to keep current format.
+
+
+> +static int snx_ser_port_total_cnt;
+> +static int snx_par_port_total_cnt;
+> +
+> +int snx_board_count;
+> +
+> +static struct snx_ser_driver sunix_ser_reg = {
+> + .dev_name = "ttySNX",
+> + .major = 0,
+> + .minor = 0,
+> + .nr = (SNX_SER_TOTAL_MAX + 1),
+> +};
+
+can't this be const ?
+
i prefer keep it in current format.
+
+> +static irqreturn_t sunix_interrupt(int irq, void *dev_id)
+> +{
+> + struct sunix_ser_port *sp = NULL;
+> + struct sunix_par_port *pp = NULL;
+> + struct sunix_board *sb = NULL;
+> + int i;
+
+> + int status = 0;
+> +
+> + int handled = IRQ_NONE;
+> +
+> + for (i = 0; i < SNX_BOARDS_MAX; i++) {
+> +
+> + if (dev_id == &(sunix_board_table[i])) {
+> + sb = dev_id;
+> + break;
+> + }
+> + }
+
+maybe put this index into the board data, so the loop on the global
+array isn't needed ?
+
i prefer keep it in current format.
+
+<snip>
+
+> + if ((sb->ser_port > 0) && (sb->ser_isr != NULL)) {
+> + sp = &sunix_ser_table[sb->ser_port_index];
+
+maybe put this pointer struct sunix_board so the lookup in the
+global array isn't necessary ?
+
i prefer keep it in current format.
+
+<snip>
+
+> + if ((sb->par_port > 0) && (sb->par_isr != NULL)) {
+> + pp = &sunix_par_table[sb->par_port_index];
+> +
+> + if (!pp)
+> + status = 1;
+> +
+> + status = sb->par_isr(sb, pp);
+> + }
+> +
+> + if (status != 0)
+> + return handled;
+> +
+> + handled = IRQ_HANDLED;
+> + return handled;
+> +}
+
+btw: is there only one irq per board or one per port ?
+
only one irq per board.
+
+> +static int snx_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+> +{
+> + return 0;
+
+shouldn't probe check whether the device is actually there and then do
+the initialization ?
+
i will fix it.
+
+<snip>
+
+> +static int snx_resume_port_termios(struct snx_ser_info *info)
+> +{
+> + struct snx_ser_state *state = NULL;
+> + struct tty_struct *tty = info->tty;
+> +
+> + state = tty->driver_data;
+> + snx_set_port_termios(state);
+> +
+> + return 0;
+> +}
+> +
+> +
+
+do we need multiple newlines here ?
+
i will fix it.
+
+<snip>
+
+> +static int snx_resume_one(struct pci_dev *pdev)
+> +{
+> + struct sunix_board *sb = pci_get_drvdata(pdev);
+> + struct sunix_ser_port *sp = NULL;
+> + int j;
+> +
+> + if (sb == NULL)
+> + return 0;
+> +
+> + for (j = 0; j < sb->ser_port; j++) {
+> + sp = &sunix_ser_table[j];
+
+can't the pointers to the serial ports be in struct sunix_board instead
+of an global array ?
+
+if it already is a global, why not statically initialized ?
+Why not doing this in snx_pci_probe() on per-board basis, and let device
+registration be done by pci subsystem ?
+
i prefer keep it in current format.
+
+> + // search sun1999 muti I/O board
+> + pdev = NULL;
+> + tablecnt = 0;
+> + sub_device_id = 0;
+> + status = 0;
+> + device_part_number = 0;
+> + bar3_base_add = 0;
+> + bar3_Byte5 = 0;
+> + bar3_Byte6 = 0;
+> + bar3_Byte7 = 0;
+> + oem_id = 0;
+> + uart_type = 0;
+> + gpio_type = 0;
+> + gpio_card_type = 0;
+> + gpio_ch_cnt = 0;
+
+completely different boards handled by one big driver ?
+why not splitting it up into multiple drivers ?
+
SUNIX has many differing cards,serail,parallel and
multi-i/o(serial + parellel), therefore, we combaine to one driver.
+
+<snip>
+
+> + if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x01))
+> + gpio_ch_cnt = 6;
+> + else if ((gpio_ch_cnt == 0x00) && (gpio_card_type == 0x02))
+> + gpio_ch_cnt = 8;
+> + else if (gpio_ch_cnt == 0x01)
+> + gpio_ch_cnt = 16;
+> + else if (gpio_ch_cnt == 0x02)
+> + gpio_ch_cnt = 32;
+> +
+
+gpio devices have their own subsystem -> therefore: write a separate
+gpio driver for that.
+
ioctl functions support for SUNIX specific serial,parellel and cash drawer
interface.
+
+<snip>
+why that limitation ? why does this have to be a global array at all ?
+
driver support maximum 4 boards can be installed incombination
(up to 32 serial port and 2 parallel port)
+
+> +static int sunix_assign_resource(void)
+> +{
+> + struct sunix_board *sb = NULL;
+> + struct sunix_ser_port *sp = NULL;
+> + struct sunix_par_port *pp = NULL;
+> +
+> + int status = 0;
+> + int i;
+> + int j;
+> + int k;
+> + int ser_n;
+> + int ser_port_index = 0;
+> +
+> + memset(sunix_ser_table, 0, (SNX_SER_TOTAL_MAX + 1) *
+> + sizeof(struct sunix_ser_port));
+> +
+> + memset(sunix_par_table, 0, (SNX_PAR_TOTAL_MAX) *
+> + sizeof(struct sunix_par_port));
+
+why this w/ global variables, instead of on per-device basis on device-
+private data ?
+
i prefer keep it in current format.
+
+<snip>
+
+> +static int sunix_ser_port_table_init(void)
+> +{
+
+same here.
+
+<snip>
+
+> +static int sunix_par_port_table_init(void)
+> +{
+
+same here
+
+> +int sunix_register_irq(void)
+> +{
+
+same here.
+
+<snip>
+why not doing those initializations on per-device basis, in the
+corresponding probe() function ?
+use devm_*() functions, which automatically releases resource when a
+device is deleted, so explicit release isn't needed anymore.
+
i will fix it.
+
+> +static void __exit snx_exit(void)
+> +{
+> + if (snx_par_port_total_cnt > 0) {
+> + sunix_par_lp_exit();
+> + sunix_par_ppdev_exit();
+> + sunix_par_parport_exit();
+> + }
+
+Let the cleanup be done by the individual driver's release functions,
+and down forget to set .owner correctly - that way, the kernel won't
+even allow unloading the module, as long as it's in use. then, you'd
+probably don't need much more than just removing unregistering the
+drivers here.
+
i prefer keep it in current format.
+
+> diff --git a/char/snx/snx_serial.c b/char/snx/snx_serial.c
+> new file mode 100644
+> index 00000000..fdac4c90
+> --- /dev/null
+> +++ b/char/snx/snx_serial.c
+> @@ -0,0 +1,4263 @@
+> +// SPDX-License-Identifier: GPL-2.0
+> +#include "snx_common.h"
+> +#include "driver_extd.h"
+> +
+> +#define SNX_ioctl_DBG 0
+
+what exactly is that ioctl for ?
+or more to the point: why are you introducing a driver specific iotctl ?
+
i will drop it.
+
+> +#define EEPROM_ACCESS_DELAY_COUNT 100000
+> +
+> +static DEFINE_SEMAPHORE(ser_port_sem);
+> +
+> +#define SNX_HIGH_BITS_OFFSET ((sizeof(long)-sizeof(int))*8)
+> +#define sunix_ser_users(state) \
+> +((state)->count + ((state)->info ? (state)->info->blocked_open : 0))
+> +
+> +static struct tty_port snx_service_port;
+
+why has this to be global, instead of in per-device private data ?
+
I am not sure what you mean ?
+
+<snip>
+
+> +static _INLINE_ void snx_ser_handle_cts_change(
+> + struct snx_ser_port *, unsigned int);
+
+Where's this strange "_INLINE_" coming from ?
+
i will fix it.
+
+<snip>
+
+> +//extern void snx_ser_change_speed(
+> + //struct snx_ser_state *state, struct SNXTERMIOS *old_termios);
+
+please no dead/commented-out code.
+
i will drop it.
+
+> +//extern int sunix_ser_interrupt(
+> + //struct sunix_board *, struct sunix_ser_port *first_sp);
+
+same here
+
i will drop it.
+
+> +//extern int sunix_ser_register_ports(struct snx_ser_driver *drv);
+> +//extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv);
+> +//extern int sunix_ser_register_driver(struct snx_ser_driver *drv);
+> +//extern void sunix_ser_unregister_driver(struct snx_ser_driver *drv);
+
+same here
+
i will drop it.
+
+> +static unsigned char READ_INTERRUPT_VECTOR_BYTE(struct sunix_ser_port *sp)
+
+i'd recommend no caps in function names.
+
i will fix it.
+
+> + if (var == 0x04) {
+> + vet4 = inb(local_vector + 0xb0);
+> + vet4 <<= 12;
+> + }
+> +
+> + data = (vet1 | vet2 | vet3 | vet4);
+> +
+> + return data;
+> + }
+> + return 0;
+> +}
+> +
+> +
+> +
+
+remove excess newlines
+
i will fix it.
+
+<snip>
+
+> +static int EEPROMWriteData(int targetConfigAddress, int address, int data)
+
+why do you use int instead of void* for addresses ?
+oh, and shouldn't it be __iomem ?
+
+targetConfigAddress for PCI (bar)base address register.
+
+<snip>
+
+> + Error = inb(targetConfigAddress + 0x08) & 0x04;
+
+<snip>
+
+> +static void snx_ser_start(struct tty_struct *tty)
+> +{
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + return;
+> +
+> + //spin_lock_irqsave(&port->lock, flags);
+> + __snx_ser_start(tty);
+> + //spin_unlock_irqrestore(&port->lock, flags);
+
+why are the spinlock calls commented out ?
+
i wiil fix it.
+
+<nsip>
+
+> + tasklet_kill(&info->tlet);
+
+what exactly is the tasklet needed for ?
+
i will fix it.
+
+> + if (!capable(CAP_SYS_ADMIN)) {
+> + retval = -EPERM;
+
+why this explicit check for CAP_SYS_ADMIN ?
+
check users with admin rights
+
+> + if (change_irq ||
+
+indention mismatch.
+
i will fix it.
+
+> + change_port ||
+
+why is userland allowed to change irq, port, etc, if that's probed
+from pci anyways ?
+
+
Since changing the 'type' of the port changes its resource
allocations, we should treat type changes the same as
IO port changes.
+
+>
+
+ (new_serial.baud_base != port->uartclk / 16) ||
+> + (close_delay != state->close_delay) ||
+> + (closing_wait != state->closing_wait) ||
+> + (new_serial.xmit_fifo_size != port->fifosize) ||
+> + (((new_flags ^ old_flags) & ~SNX_UPF_USR_MASK) != 0)) {
+> + goto exit;
+> + }
+> +
+> + port->flags = ((port->flags & ~SNX_UPF_USR_MASK) |
+> + (new_flags & SNX_UPF_USR_MASK));
+> + port->custom_divisor = new_serial.custom_divisor;
+> + goto check_and_exit;
+> + }
+
+<snip>
+
+> +static int snx_ser_ioctl(struct tty_struct *tty,
+> +unsigned int cmd, unsigned long arg)
+> +{
+<snip>
+> + case TIOCSSERIAL:
+> + {
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + state->port->setserial_flag = SNX_SER_BAUD_SETSERIAL;
+> + ret = snx_ser_set_info(state,
+> + (struct serial_struct *)arg);
+> + }
+> + break;
+> + }
+> +
+> +
+> + case TCSETS:
+> + {
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + state->port->flags &= ~(SNX_UPF_SPD_HI |
+> + SNX_UPF_SPD_VHI |
+> + SNX_UPF_SPD_SHI |
+> + SNX_UPF_SPD_WARP);
+> + state->port->setserial_flag = SNX_SER_BAUD_NOTSETSER;
+> + snx_ser_update_termios(state);
+> + }
+> + break;
+> + }
+> +
+
+Why do you need your own implementation of these tty ioctl()'s ?
+The tty subsystem handles them on its own (using the callbacks for hw-
+specific operations)
+
ioctl functions support for SUNIX specific serial,parellel and cash drawer interface.
+
+> + case SNX_SER_DUMP_PORT_INFO:
+<snip>
+> + case SNX_SER_DUMP_DRIVER_VER:
+<snip>
+> + case SNX_COMM_GET_BOARD_CNT:
+<snip>
+> + case SNX_COMM_GET_BOARD_INFO:
+
+is it really necessary to introduce your own driver-specific ioctl() ?
+why not putting these things into debugfs or sysfs ?
+
ioctl functions support for SUNIX specific serial,
parellel and cash drawer interface.
+
+> + case SNX_GPIO_GET:
+<snip>
+> + case SNX_GPIO_SET:
+<snip>
+> + case SNX_GPIO_READ:
+<snip>
+> + case SNX_GPIO_WRITE:
+<snip>
+> + case SNX_GPIO_SET_DEFAULT:
+<snip>
+> + case SNX_GPIO_WRITE_DEFAULT:
+<snip>
+> + case SNX_GPIO_GET_INPUT_INVERT:
+<snip>
+> + case SNX_GPIO_SET_INPUT_INVERT:
+
+
+gpio stuff clearly doesn't belong into tty ioctl()s.
+
+that's what the gpio subsystem is there for - this provides the linux
+standard api for gpio access.
+
+> + case SNX_UART_GET_TYPE:
+
+what exactly is this for ?
+
+<snip>
+> + } else {
+> + //pr_err(CE_NOTE, "WARNING : incorrect port
+> + //number (port = %d)!",gb.uart_num);
+> + break;
+> + }
+> +
+> + switch (uart_type) {
+> + case 0: // RS-232
+
+yet another indention mismatch.
+and please remove dead code.
+
i will fix it.
+
+<snip>
+
+> + case SNX_UART_SET_TYPE: {
+
+what is this for ?
+
+<snip>
+
+> + case SNX_UART_SET_ACS:
+
+what is "ACS" ?
+
RS485 ACS Enable,Enable SUNIX UART Controller waiting transmit
data when receive data is going.
+
+> + {
+> + SNX_DRVR_UART_SET_ACS gb;
+> + struct sunix_board *sb = NULL;
+> + int ACSstate = 0;
+> + int targetConfigAddress = 0;
+> +
+> + memset(&gb, 0, sizeof(SNX_DRVR_UART_SET_ACS));
+> +
+> + if (copy_from_user(&gb, (void *)arg,
+> + (sizeof(SNX_DRVR_UART_SET_ACS))))
+> + ret = -EFAULT;
+> + else
+> + ret = 0;
+> +
+> + sb = &sunix_board_table[gb.board_id - 1];
+
+do we really need to access global variables here ?
+
i prefer to keep current format.
+
+<snip>
+
+> + if (tty->flags & (1 << TTY_IO_ERROR)) {
+> + ret = -EIO;
+> + goto out;
+> + }
+> +
+> + switch (cmd) {
+> + case TIOCMIWAIT:
+
+yet another idention mismatch.
+
i will fix it.
+
+<snip>
+
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + down(&state->sem);
+> +
+> + switch (cmd) {
+> + case TIOCSERGETLSR:
+> + ret = snx_ser_get_lsr_info(state, (unsigned int *)arg);
+> + break;
+> +
+> + default:
+> + {
+> + break;
+> + }
+> + }
+> +
+> + up(&state->sem);
+> + }
+
+even more indention mismatches.
+
i will fix it.
+
+<snip>
+
+> +static void snx_ser_set_termios(struct tty_struct *tty,
+> +struct SNXTERMIOS *old_termios)
+> +{
+> + struct snx_ser_state *state = NULL;
+> + unsigned long flags;
+> + unsigned int cflag = tty->termios.c_cflag;
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + return;
+> +
+> + state = tty->driver_data;
+> +
+> +#define RELEVANT_IFLAG(iflag) ((iflag) & (IGNBRK|BRKINT|IGNPAR|PARMRK|INPCK))
+
+please no #define's in the middle of a function.
+
i will fix it.
+
+<snip>
+
+> +static void snx_ser_update_timeout(struct snx_ser_port *port,
+> +unsigned int cflag, unsigned int baud)
+> +{
+> + unsigned int bits;
+> +
+> + switch (cflag & CSIZE) {
+> + case CS5:
+> + bits = 7;
+> + break;
+> +
+
+more indention mismatches.
+
i will fix it.
+
+<snip>
+
+> +static int snx_ser_open(struct tty_struct *tty, struct file *filp)
+> +{
+> + struct snx_ser_driver *drv =
+> + (struct snx_ser_driver *)tty->driver->driver_state;
+> +
+> + struct snx_ser_state *state = NULL;
+> + struct tty_port *tport = NULL;
+> +
+> + int retval = 0;
+> + int line = SNX_SER_DEVNUM(tty);
+> +
+> + if (line < SNX_SER_TOTAL_MAX) {
+> + retval = -ENODEV;
+> +
+> + if (line >= SNX_SER_TOTAL_MAX)
+> + goto fail;
+> +
+> + state = snx_ser_get(drv, line);
+> +
+> + tport = &state->tport;
+> +
+> + if (IS_ERR(state)) {
+> + retval = PTR_ERR(state);
+> + goto fail;
+> + }
+> +
+> + if (!state)
+> + goto fail;
+> +
+> +
+> + state->port->suspended = 1;
+> + tty->driver_data = state;
+> +
+> + tport->low_latency = (state->port->flags &
+> + SNX_UPF_LOW_LATENCY) ? 1 : 0;
+> +
+> + state->info->tty = tty;
+> +
+> + tty_port_tty_set(tport, tty);
+> +
+> + if (tty_hung_up_p(filp)) {
+> + retval = -EAGAIN;
+> + state->count--;
+> + up(&state->sem);
+> + goto fail;
+> + }
+> +
+> + retval = snx_ser_startup(state, 0);
+> +
+> + if (retval == 0)
+> + retval = snx_ser_block_til_ready(filp, state);
+> +
+> + up(&state->sem);
+> +
+> + if (retval == 0 && !(state->info->flags &
+> + SNX_UIF_NORMAL_ACTIVE)) {
+> + state->info->flags |= SNX_UIF_NORMAL_ACTIVE;
+> +
+> + snx_ser_update_termios(state);
+> + }
+> +
+> + try_module_get(THIS_MODULE);
+
+why this ?
+
i will fix it.
+
+<snip>
+
+> +extern int sunix_ser_register_driver(struct snx_ser_driver *drv)
+> +{
+> + struct tty_driver *normal = NULL;
+> + int i;
+> + int ret = 0;
+> +
+> + drv->state = kmalloc(sizeof(struct snx_ser_state) * drv->nr,
+> + GFP_KERNEL);
+> +
+> + ret = -ENOMEM;
+> +
+> + if (!drv->state) {
+> + pr_err("SNX Error: Allocate memory fail !\n\n");
+> + goto out;
+> + }
+> +
+> + memset(drv->state, 0, sizeof(struct snx_ser_state) * drv->nr);
+> +
+> + for (i = 0; i < drv->nr; i++) {
+> + struct snx_ser_state *state = drv->state + i;
+> + struct tty_port *tport = &state->tport;
+> +
+> + tty_port_init(tport);
+
+does that really need to be globally in driver init, instead of in per
+port device->open (and use device's private data) ?
+
i prefer to keep current format.
+
+<snip>
+
+> +extern void sunix_ser_unregister_ports(struct snx_ser_driver *drv)
+
+why are these 'extern' ?!
+
functions definition in multiple .c files.
\ No newline at end of file
--
2.17.1
Powered by blists - more mailing lists