| 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: <2863fe62-9724-a2d7-f4f8-6a74648afd0a@metux.net>
Date: Fri, 8 Mar 2019 21:54:54 +0100
From: "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
To: Morris Ku <saumah@...il.com>, gregkh@...uxfoundation.org
Cc: morris_ku@...ix.com, linux-kernel@...r.kernel.org, arnd@...db.de
Subject: Re: [PATCH 3/4] Add header file,Kconfig and Makefile
On 08.03.19 13:35, Morris Ku wrote:
> This patch add header file, Kconfig and Makefile.
>
> Signed-off-by: Morris Ku <saumah@...il.com>
> ---
> char/snx/Kconfig | 15 +
> char/snx/Makefile | 9 +
> char/snx/driver_extd.h | 170 ++++++
> char/snx/snx_common.h | 1157 ++++++++++++++++++++++++++++++++++++++++
> char/snx/snx_lp.h | 126 +++++
> char/snx/snx_ppdev.h | 43 ++
why isn't that in ./drivers/tty/serial/sunix/ ?
<snip>
> diff --git a/char/snx/Kconfig b/char/snx/Kconfig
> new file mode 100644
> index 00000000..86f38352
> --- /dev/null
> +++ b/char/snx/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Character device configuration
> +#
> +
> +config SNX
please use full name: SUNIX
<snip>
> diff --git a/char/snx/driver_extd.h b/char/snx/driver_extd.h
> new file mode 100644
> index 00000000..27f69570
> --- /dev/null
> +++ b/char/snx/driver_extd.h
> @@ -0,0 +1,170 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _SNXHW_DRVR_EXTR_H_
> +#define _SNXHW_DRVR_EXTR_H_
> +
> +#ifndef SNX_IOCTL
> +#define SNX_IOCTL 0x900
> +#endif
> +
> +#define SNX_COMM_GET_BOARD_CNT (SNX_IOCTL + 100)
> +#define SNX_COMM_GET_BOARD_INFO (SNX_IOCTL + 101)
> +
> +#define SNX_GPIO_GET (SNX_IOCTL + 200)
> +#define SNX_GPIO_SET (SNX_IOCTL + 201)
> +#define SNX_GPIO_READ (SNX_IOCTL + 202)
> +#define SNX_GPIO_WRITE (SNX_IOCTL + 203)
> +#define SNX_GPIO_SET_DEFAULT (SNX_IOCTL + 204)
> +#define SNX_GPIO_WRITE_DEFAULT (SNX_IOCTL + 205)
> +#define SNX_GPIO_GET_INPUT_INVERT (SNX_IOCTL + 206)
> +#define SNX_GPIO_SET_INPUT_INVERT (SNX_IOCTL + 207)
> +
> +#define SNX_UART_GET_TYPE (SNX_IOCTL + 300)
> +#define SNX_UART_SET_TYPE (SNX_IOCTL + 301)
> +#define SNX_UART_GET_ACS (SNX_IOCTL + 302)
> +#define SNX_UART_SET_ACS (SNX_IOCTL + 303)
why exactly do you introduce driver-specific ioctls ?
what is "ACS"
> +#define SNX_GPIO_IN 0
> +#define SNX_GPIO_OUT 1
> +#define SNX_GPIO_LOW 0
> +#define SNX_GPIO_HI 1
> +#define SNX_GPIO_INPUT_INVERT_D 0
> +#define SNX_GPIO_INPUT_INVERT_E 1
GPIO stuff belongs into the gpio subsystem. see drivers/gpio/
<snip>
> diff --git a/char/snx/snx_common.h b/char/snx/snx_common.h
> new file mode 100644
> index 00000000..16dd8f02
> --- /dev/null
> +++ b/char/snx/snx_common.h
> @@ -0,0 +1,1157 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#ifdef MODVERSIONS
> +#ifndef MODULE
> +#define MODULE
> +#endif
> +#endif
dont need that. please drop it.
<snip>
> +#ifndef KERNEL_VERSION
> +#define KERNEL_VERSION(ver, rel, seq) ((ver << 16) | (rel << 8) | seq)
> +#endif
same here.
> +#include <linux/errno.h>
> +#include <linux/signal.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +#include <linux/serial.h>
> +#include <linux/serial_reg.h>
> +#include <linux/ioport.h>
> +#include <linux/mm.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/wait.h>
> +#include <linux/tty_driver.h>
> +#include <linux/pci.h>
> +#include <linux/circ_buf.h>
> +#include <asm/uaccess.h>
> +#include <asm/io.h>
> +#include <asm/irq.h>
> +#include <asm/segment.h>
> +#include <asm/serial.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/workqueue.h>
> +#include <linux/moduleparam.h>
> +#include <linux/console.h>
> +#include <linux/sysrq.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/kref.h>
> +#include <linux/parport.h>
> +#include <linux/ctype.h>
> +#include <linux/poll.h>
> +#include <linux/sched.h>
> +#include <linux/serial_8250.h>
> +#include <linux/cdev.h>
> +#include <linux/sched/signal.h>
are these really all needed within the header ?
> +extern int snx_board_count;
Why that extern field ?
> +#define SNX_DRIVER_VERSION "V2.0.4.5"
> +#define SNX_DRIVER_AUTHOR "SUNIX Co., Ltd.<info@...ix.com.tw>"
> +#define SNX_DRIVER_DESC "SUNIX Multi-I/O Board Driver Module"
Does it need to be in here ? (see MODULE_*() macros)
<snip>
> +typedef struct _PORT {
> + char type;
> +
> + int bar1;
> + unsigned char offset1;
> + unsigned char length1;
> +
> + int bar2;
> + unsigned char offset2;
> + unsigned char length2;
> +
> + unsigned int intmask;
> + unsigned int chip_flag;
> +
> +} PORT;
please no uppercase type names, and use proper prefix.
> +#if defined(__i386__) && (defined(CONFIG_M386) || \
> +defined(CONFIG_M486))
> +#define SNX_SERIAL_INLINE
> +#endif
> +
> +#ifdef SNX_SERIAL_INLINE
> +#define _INLINE_ inline
> +#else
> +#define _INLINE_
> +#endif
what is that for ?!
> +struct snx_ser_driver {
> + struct module *owner;
> + const char *driver_name;
> + const char *dev_name;
> + int major;
> + int minor;
> + int nr;
> + struct snx_ser_state *state;
> + struct tty_driver *tty_driver;
> +};
oh, not good. the struct tty_driver should be contained in
snx_ser_driver (not a pointer to it). or the other way round:
give the tty driver a pointer to a driver-private struct.
and this data looks as it should be assigned to individual
devices, not to driver globally.
> +#include <linux/tty_flip.h>
put all includes together at the top
> +static inline void snx_ser_insert_char
> +(
> + struct snx_ser_port *port,
> + unsigned int status,
> + unsigned int overrun,
> + unsigned int ch,
> + unsigned int flag
> +)
> +{
> + struct snx_ser_info *info = port->info;
> + struct tty_struct *tty = info->tty;
> + struct snx_ser_state *state = NULL;
> + struct tty_port *tport = NULL;
> +
> + state = tty->driver_data;
> +
> + tport = &state->tport;
> +
> + if ((status & port->ignore_status_mask & ~overrun) == 0) {
> +
> + if (tty_insert_flip_char(tport, ch, flag) == 0)
> + ++port->icount.buf_overrun;
> + }
> +
> + if (status & ~port->ignore_status_mask & overrun) {
> +
> + if (tty_insert_flip_char(tport, 0, TTY_OVERRUN) == 0)
> + ++port->icount.buf_overrun;
> + }
> +}
too big for an inline function.
> +#define SNX_CONFIG_PARPORT_1284
> +#define SNX_CONFIG_PARPORT_PC_FIFO
> +
> +#ifdef SNX_CONFIG_PARPORT_1284
> +#undef SNX_CONFIG_PARPORT_1284
> +#endif
> +
> +#ifdef SNX_CONFIG_PARPORT_PC_FIFO
> +#undef SNX_CONFIG_PARPORT_PC_FIFO
> +#endif
parport, serial port, gpio should be separate drivers.
> +struct snx_parport_ops {
Why does the driver introduce its own callback vectors ?
> +
> +struct sunix_par_port {
> + struct snx_parport *port;
> +
> + unsigned char ctr;
> + unsigned char ctr_writable;
> + int ecr;
> + int fifo_depth;
> + int pword;
> + int read_intr_threshold;
> + int write_intr_threshold;
> +
> + char *dma_buf;
why not void * ?
> + dma_addr_t dma_handle;
> + struct list_head list;
> +
> + unsigned long base;
> + unsigned long base_hi;
> + int irq;
> + int portnum;
> + unsigned int snx_type;
> +
> + int board_enum;
> + unsigned int bus_number;
> + unsigned int dev_number;
> +
> + PCI_BOARD pb_info;
> +
> + unsigned char chip_flag;
> + unsigned int port_flag;
> +};
> +
> +// snx_devtable.c
> +extern PCI_BOARD snx_pci_board_conf[];
<snip>
why all these extern functions ?
<snip>
> +extern char snx_ser_ic_table[SNX_SER_PORT_MAX_UART][10];
> +extern char snx_par_ic_table[SNX_PAR_PORT_MAX_UART][10];
> +extern char snx_port_remap[2][10];
please try not to use global fields. these things look they belong into
the corresponding device private data structs.
> +#define sunix_parport_write_data(p, x) sunix_parport_pc_write_data(p, x)
> +#define sunix_parport_read_data(p) sunix_parport_pc_read_data(p)
> +#define sunix_parport_write_control(p, x) \
> +sunix_parport_pc_write_control(p, x)
> +#define sunix_parport_read_control(p) sunix_parport_pc_read_control(p)
> +#define sunix_parport_frob_control(p, m, v) \
> +sunix_parport_pc_frob_control(p, m, v)
> +#define sunix_parport_read_status(p) \
> +sunix_parport_pc_read_status(p)
> +#define sunix_parport_enable_irq(p) sunix_parport_pc_enable_irq(p)
> +#define sunix_parport_disable_irq(p) sunix_parport_pc_disable_irq(p)
> +#define sunix_parport_data_forward(p) sunix_parport_pc_data_forward(p)
> +#define sunix_parport_data_reverse(p) sunix_parport_pc_data_reverse(p)
does that really need to be in a header file ?
<snip>
> +static inline unsigned char sunix_parport_pc_frob_control(
> +struct snx_parport *p, unsigned char mask, unsigned char val)
> +{
> + const unsigned char wm = (PARPORT_CONTROL_STROBE |
> + PARPORT_CONTROL_AUTOFD |
> + PARPORT_CONTROL_INIT |
> + PARPORT_CONTROL_SELECT);
> +
> + if (mask & 0x20) {
> + if (val & 0x20)
> + sunix_parport_pc_data_reverse(p);
> + else
> + sunix_parport_pc_data_forward(p);
> +
> + }
> +
> + mask &= wm;
> + val &= wm;
> +
> + return __sunix_parport_pc_frob_control(p, mask, val);
> +}
do these functions really *need* to be in the header and static inline ?
(IOW: are they really needed from multiple .c files ?)
> diff --git a/char/snx/snx_lp.h b/char/snx/snx_lp.h
> new file mode 100644
> index 00000000..8c48deea
> --- /dev/null
> +++ b/char/snx/snx_lp.h
<snip>
> +#define __KERNEL__ 1
> +
> +#ifdef __KERNEL__
doesn't belong here. drop that.
> +
> +#include <linux/mutex.h>
put this at the top. if it's really needed here.
> diff --git a/char/snx/snx_ppdev.h b/char/snx/snx_ppdev.h
> new file mode 100644
> index 00000000..635f99e1
> --- /dev/null
> +++ b/char/snx/snx_ppdev.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include "snx_common.h"
> +
> +#define SNX_PP_IOCTL 'p'
> +
> +struct snx_ppdev_frob_struct {
> + unsigned char mask;
> + unsigned char val;
> +};
> +
> +
> +#define SNX_PPSETMODE _IOW(SNX_PP_IOCTL, 0x80, int)
> +#define SNX_PPRSTATUS _IOR(SNX_PP_IOCTL, 0x81, unsigned char)
> +#define SNX_PPRCONTROL _IOR(SNX_PP_IOCTL, 0x83, unsigned char)
> +#define SNX_PPWCONTROL _IOW(SNX_PP_IOCTL, 0x84, unsigned char)
> +#define SNX_PPFCONTROL _IOW(SNX_PP_IOCTL, 0x8e,\
> +struct snx_ppdev_frob_struct)
> +#define SNX_PPRDATA _IOR(SNX_PP_IOCTL, 0x85, unsigned char)
> +#define SNX_PPWDATA _IOW(SNX_PP_IOCTL, 0x86, unsigned char)
> +#define SNX_PPCLAIM _IO(SNX_PP_IOCTL, 0x8b)
> +#define SNX_PPRELEASE _IO(SNX_PP_IOCTL, 0x8c)
> +#define SNX_PPYIELD _IO(SNX_PP_IOCTL, 0x8d)
> +#define SNX_PPEXCL _IO(SNX_PP_IOCTL, 0x8f)
> +#define SNX_PPDATADIR _IOW(SNX_PP_IOCTL, 0x90, int)
> +#define SNX_PPNEGOT _IOW(SNX_PP_IOCTL, 0x91, int)
> +#define SNX_PPWCTLONIRQ _IOW(SNX_PP_IOCTL, 0x92, unsigned char)
> +#define SNX_PPCLRIRQ _IOR(SNX_PP_IOCTL, 0x93, int)
> +#define SNX_PPSETPHASE _IOW(SNX_PP_IOCTL, 0x94, int)
> +#define SNX_PPGETTIME _IOR(SNX_PP_IOCTL, 0x95, struct timeval)
> +#define SNX_PPSETTIME _IOW(SNX_PP_IOCTL, 0x96, struct timeval)
> +#define SNX_PPGETMODES _IOR(SNX_PP_IOCTL, 0x97, unsigned int)
> +#define SNX_PPGETMODE _IOR(SNX_PP_IOCTL, 0x98, int)
> +#define SNX_PPGETPHASE _IOR(SNX_PP_IOCTL, 0x99, int)
> +#define SNX_PPGETFLAGS _IOR(SNX_PP_IOCTL, 0x9a, int)
> +#define SNX_PPSETFLAGS _IOW(SNX_PP_IOCTL, 0x9b, int)
> +
> +#define SNX_PP_FASTWRITE (1<<2)
> +#define SNX_PP_FASTREAD (1<<3)
> +#define SNX_PP_W91284PIC (1<<4)
> +
> +#define SNX_PP_FLAGMASK (SNX_PP_FASTWRITE | SNX_PP_FASTREAD \
what exactly are these needed for ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@...ux.net -- +49-151-27565287
Powered by blists - more mailing lists