Hi,
This set of patches adds support for the ISO7816 standard. The USART devices in
Microchip SoCs have an ISO7816 mode. It allows to let the USART managing
the CLK and I/O signals of a smart card.
Nicolas Ferre (3):
tty/serial_core: add ISO7816 infrastructure
tty/serial: atmel: add ISO7816 support
tty/serial: atmel: manage shutdown in case of RS485 or ISO7816 mode
drivers/tty/serial/atmel_serial.c | 179 +++++++++++++++++++++++++++++++++++---
drivers/tty/serial/atmel_serial.h | 3 +-
drivers/tty/serial/serial_core.c | 49 +++++++++++
include/linux/serial_core.h | 3 +
include/uapi/asm-generic/ioctls.h | 2 +
include/uapi/linux/serial.h | 17 ++++
6 files changed, 241 insertions(+), 12 deletions(-)
--
2.12.2
From: Nicolas Ferre <[email protected]>
In atmel_shutdown() we call atmel_stop_rx() and atmel_stop_tx() functions.
Prevent the rx restart that is implemented in RS485 or ISO7816 modes when
calling atmel_stop_tx() by using the atomic information tasklet_shutdown
that is already in place for this purpose.
Signed-off-by: Nicolas Ferre <[email protected]>
---
drivers/tty/serial/atmel_serial.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 0118b219f3a8..e4f877e1f3c6 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -617,7 +617,9 @@ static void atmel_stop_tx(struct uart_port *port)
if (((port->rs485.flags & SER_RS485_ENABLED) &&
!(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
port->iso7816.flags & SER_ISO7816_ENABLED)
- atmel_start_rx(port);
+ if (!atomic_read(&atmel_port->tasklet_shutdown))
+ atmel_start_rx(port);
+
}
/*
--
2.12.2
From: Nicolas Ferre <[email protected]>
When mode is set in atmel_config_iso7816() we backup last RS232 mode
for coming back to this mode if requested.
Also allow setup of T=0 and T=1 parameter and basic support in set_termios
function as well.
Report NACK and ITER errors in irq handler.
Signed-off-by: Nicolas Ferre <[email protected]>
[[email protected]: rebase, add check on fidi ratio, checkpatch fixes]
Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/tty/serial/atmel_serial.c | 175 +++++++++++++++++++++++++++++++++++---
drivers/tty/serial/atmel_serial.h | 3 +-
2 files changed, 167 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 8e4428725848..0118b219f3a8 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -34,6 +34,7 @@
#include <linux/suspend.h>
#include <linux/mm.h>
+#include <asm/div64.h>
#include <asm/io.h>
#include <asm/ioctls.h>
@@ -147,6 +148,8 @@ struct atmel_uart_port {
struct circ_buf rx_ring;
struct mctrl_gpios *gpios;
+ u32 backup_mode; /* MR saved during iso7816 operations */
+ u32 backup_brgr; /* BRGR saved during iso7816 operations */
unsigned int tx_done_mask;
u32 fifo_size;
u32 rts_high;
@@ -362,6 +365,136 @@ static int atmel_config_rs485(struct uart_port *port,
return 0;
}
+static unsigned int atmel_calc_cd(struct uart_port *port,
+ struct serial_iso7816 *iso7816conf)
+{
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ unsigned int cd;
+ u64 mck_rate;
+
+ mck_rate = (u64)clk_get_rate(atmel_port->clk);
+ dev_dbg(port->dev, "ISO7816 mck_rate = %lld\n", mck_rate);
+ do_div(mck_rate, iso7816conf->clk);
+ cd = mck_rate;
+ dev_dbg(port->dev, "ISO7816 clk = %d. CD = %d\n", iso7816conf->clk, cd);
+ return cd;
+}
+
+static unsigned int atmel_calc_fidi(struct uart_port *port,
+ struct serial_iso7816 *iso7816conf)
+{
+ u64 fidi = 0;
+
+ dev_dbg(port->dev, "ISO7816 fi(%d) / di(%d)\n",
+ iso7816conf->sc_fi, iso7816conf->sc_di);
+ if (iso7816conf->sc_fi && iso7816conf->sc_di) {
+ fidi = (u64)iso7816conf->sc_fi;
+ do_div(fidi, iso7816conf->sc_di);
+ }
+ dev_dbg(port->dev, "ISO7816 fidi = 0x%x\n", (u32)fidi);
+ return (u32)fidi;
+}
+
+/* Enable or disable the iso7816 support */
+/* Called with interrupts disabled */
+static int atmel_config_iso7816(struct uart_port *port,
+ struct serial_iso7816 *iso7816conf)
+{
+ struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ unsigned int mode, t;
+ unsigned int cd, fidi;
+ int ret = 0;
+
+ /* Disable RX and TX */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXDIS | ATMEL_US_TXDIS);
+ /* Disable interrupts */
+ atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
+
+ mode = atmel_uart_readl(port, ATMEL_US_MR);
+
+ if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) {
+ /* port not yet in iso7816 mode: store configuration */
+ atmel_port->backup_mode = mode;
+ atmel_port->backup_brgr = atmel_uart_readl(port, ATMEL_US_BRGR);
+ }
+
+ if (iso7816conf->flags & SER_ISO7816_ENABLED) {
+ mode &= ~ATMEL_US_USMODE;
+
+ if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(0)) {
+ mode |= ATMEL_US_USMODE_ISO7816_T0;
+ t = 0;
+ } else if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(1)) {
+ mode |= ATMEL_US_USMODE_ISO7816_T1;
+ t = 1;
+ } else {
+ dev_warn(port->dev, "ISO7816 Type not supported. Resetting\n");
+ memset(iso7816conf, 0, sizeof(struct serial_iso7816));
+ goto err_out;
+ }
+
+ dev_dbg(port->dev, "Setting USART to ISO7816 mode T%d\n", t);
+
+ mode &= ~(ATMEL_US_USCLKS | ATMEL_US_CHRL
+ | ATMEL_US_NBSTOP | ATMEL_US_PAR);
+
+ /* NACK configuration */
+ if ((iso7816conf->flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(0))
+ mode |= ATMEL_US_DSNACK;
+ else
+ mode |= ATMEL_US_INACK;
+ /* select mck clock, and output */
+ mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
+ /* set parity for normal/inverse mode + max iterations */
+ mode |= ATMEL_US_PAR_EVEN | ATMEL_US_NBSTOP_1 | (3 << 24);
+
+ cd = atmel_calc_cd(port, iso7816conf);
+ fidi = atmel_calc_fidi(port, iso7816conf);
+ if (fidi < 0) {
+ dev_warn(port->dev, "ISO7816 fidi = 0, Generator generates no signal\n");
+ } else if (fidi == 1 || fidi == 2) {
+ dev_err(port->dev, "ISO7816 fidi = %u, value not supported\n", fidi);
+ ret = -EINVAL;
+ goto err_out;
+ }
+ /* Actually set ISO7816 mode */
+ atmel_uart_writel(port, ATMEL_US_TTGR, iso7816conf->tg);
+ atmel_uart_writel(port, ATMEL_US_BRGR, cd);
+ atmel_uart_writel(port, ATMEL_US_FIDIR, fidi);
+
+ atmel_port->tx_done_mask = ATMEL_US_TXEMPTY | ATMEL_US_NACK | ATMEL_US_ITERATION;
+ } else {
+ dev_dbg(port->dev, "Setting UART to RS232\n");
+ /* back to last RS232 settings */
+ mode = atmel_port->backup_mode;
+ memset(iso7816conf, 0, sizeof(struct serial_iso7816));
+ atmel_uart_writel(port, ATMEL_US_TTGR, 0);
+ atmel_uart_writel(port, ATMEL_US_BRGR, atmel_port->backup_brgr);
+ atmel_uart_writel(port, ATMEL_US_FIDIR, 0x174);
+
+ if (atmel_use_pdc_tx(port))
+ atmel_port->tx_done_mask = ATMEL_US_ENDTX |
+ ATMEL_US_TXBUFE;
+ else
+ atmel_port->tx_done_mask = ATMEL_US_TXRDY;
+ }
+
+ port->iso7816 = *iso7816conf;
+
+ atmel_uart_writel(port, ATMEL_US_MR, mode);
+
+err_out:
+ /* Enable interrupts */
+ atmel_uart_writel(port, ATMEL_US_IER, atmel_port->tx_done_mask);
+ /* Enable RX and TX */
+ atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXEN | ATMEL_US_TXEN);
+
+ return ret;
+}
+
/*
* Return TIOCSER_TEMT when transmitter FIFO and Shift register is empty.
*/
@@ -481,8 +614,9 @@ static void atmel_stop_tx(struct uart_port *port)
/* Disable interrupts */
atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED)
atmel_start_rx(port);
}
@@ -500,8 +634,9 @@ static void atmel_start_tx(struct uart_port *port)
return;
if (atmel_use_pdc_tx(port) || atmel_use_dma_tx(port))
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED)
atmel_stop_rx(port);
if (atmel_use_pdc_tx(port))
@@ -799,8 +934,9 @@ static void atmel_complete_tx_dma(void *arg)
*/
if (!uart_circ_empty(xmit))
atmel_tasklet_schedule(atmel_port, &atmel_port->tasklet_tx);
- else if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+ else if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED) {
/* DMA done, stop TX, start RX for RS485 */
atmel_start_rx(port);
}
@@ -1281,6 +1417,9 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
wake_up_interruptible(&port->state->port.delta_msr_wait);
}
}
+
+ if (pending & (ATMEL_US_NACK | ATMEL_US_ITERATION))
+ dev_err(port->dev, "ISO7816 ERROR (0x%08x)\n", pending);
}
/*
@@ -1373,8 +1512,9 @@ static void atmel_tx_pdc(struct uart_port *port)
atmel_uart_writel(port, ATMEL_US_IER,
atmel_port->tx_done_mask);
} else {
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
+ if (((port->rs485.flags & SER_RS485_ENABLED) &&
+ !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
+ port->iso7816.flags & SER_ISO7816_ENABLED) {
/* DMA done, stop TX, start RX for RS485 */
atmel_start_rx(port);
}
@@ -2099,6 +2239,18 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
atmel_uart_writel(port, ATMEL_US_TTGR,
port->rs485.delay_rts_after_send);
mode |= ATMEL_US_USMODE_RS485;
+ } else if (port->iso7816.flags & SER_ISO7816_ENABLED) {
+ dev_dbg(port->dev, "Setting UART to ISO7816 in termios\n");
+ atmel_uart_writel(port, ATMEL_US_TTGR, port->iso7816.tg);
+ /* select mck clock, and output */
+ mode |= ATMEL_US_USCLKS_MCK | ATMEL_US_CLKO;
+ /* set max iterations */
+ mode |= (3 << 24);
+ if ((port->iso7816.flags & SER_ISO7816_T_PARAM)
+ == SER_ISO7816_T(0))
+ mode |= ATMEL_US_USMODE_ISO7816_T0;
+ else
+ mode |= ATMEL_US_USMODE_ISO7816_T1;
} else if (termios->c_cflag & CRTSCTS) {
/* RS232 with hardware handshake (RTS/CTS) */
if (atmel_use_fifo(port) &&
@@ -2175,7 +2327,8 @@ static void atmel_set_termios(struct uart_port *port, struct ktermios *termios,
}
quot = cd | fp << ATMEL_US_FP_OFFSET;
- atmel_uart_writel(port, ATMEL_US_BRGR, quot);
+ if (!(port->iso7816.flags & SER_ISO7816_ENABLED))
+ atmel_uart_writel(port, ATMEL_US_BRGR, quot);
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA | ATMEL_US_RSTRX);
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
atmel_port->tx_stopped = false;
@@ -2355,6 +2508,7 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
port->mapbase = pdev->resource[0].start;
port->irq = pdev->resource[1].start;
port->rs485_config = atmel_config_rs485;
+ port->iso7816_config = atmel_config_iso7816;
port->membase = NULL;
memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
@@ -2379,7 +2533,8 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
}
/* Use TXEMPTY for interrupt when rs485 else TXRDY or ENDTX|TXBUFE */
- if (port->rs485.flags & SER_RS485_ENABLED)
+ if (port->rs485.flags & SER_RS485_ENABLED ||
+ port->iso7816.flags & SER_ISO7816_ENABLED)
atmel_port->tx_done_mask = ATMEL_US_TXEMPTY;
else if (atmel_use_pdc_tx(port)) {
port->fifosize = PDC_BUFFER_SIZE;
diff --git a/drivers/tty/serial/atmel_serial.h b/drivers/tty/serial/atmel_serial.h
index ba3a2437cde4..fff51f5fe8bc 100644
--- a/drivers/tty/serial/atmel_serial.h
+++ b/drivers/tty/serial/atmel_serial.h
@@ -124,7 +124,8 @@
#define ATMEL_US_TTGR 0x28 /* Transmitter Timeguard Register */
#define ATMEL_US_TG GENMASK(7, 0) /* Timeguard Value */
-#define ATMEL_US_FIDI 0x40 /* FI DI Ratio Register */
+#define ATMEL_US_FIDIR 0x40 /* FI DI Ratio Register */
+#define ATMEL_US_FIDI GENMASK(15, 0) /* FIDI ratio */
#define ATMEL_US_NER 0x44 /* Number of Errors Register */
#define ATMEL_US_IF 0x4c /* IrDA Filter Register */
--
2.12.2
From: Nicolas Ferre <[email protected]>
Add the ISO7816 ioctl and associated accessors and data structure.
Drivers can then use this common implementation to handle ISO7816.
Signed-off-by: Nicolas Ferre <[email protected]>
[[email protected]: squash and rebase, removal of gpios, checkpatch fixes]
Signed-off-by: Ludovic Desroches <[email protected]>
---
drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++++++++++++++++++++
include/linux/serial_core.h | 3 +++
include/uapi/asm-generic/ioctls.h | 2 ++
include/uapi/linux/serial.h | 17 ++++++++++++++
4 files changed, 71 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c14a453f73c..c89ac59f6f8c 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port *port,
return 0;
}
+static int uart_get_iso7816_config(struct uart_port *port,
+ struct serial_iso7816 __user *iso7816)
+{
+ unsigned long flags;
+ struct serial_iso7816 aux;
+
+ spin_lock_irqsave(&port->lock, flags);
+ aux = port->iso7816;
+ spin_unlock_irqrestore(&port->lock, flags);
+
+ if (copy_to_user(iso7816, &aux, sizeof(aux)))
+ return -EFAULT;
+
+ return 0;
+}
+
+static int uart_set_iso7816_config(struct uart_port *port,
+ struct serial_iso7816 __user *iso7816_user)
+{
+ struct serial_iso7816 iso7816;
+ int ret;
+ unsigned long flags;
+
+ if (!port->iso7816_config)
+ return -ENOIOCTLCMD;
+
+ if (copy_from_user(&iso7816, iso7816_user, sizeof(*iso7816_user)))
+ return -EFAULT;
+
+ spin_lock_irqsave(&port->lock, flags);
+ ret = port->iso7816_config(port, &iso7816);
+ spin_unlock_irqrestore(&port->lock, flags);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(iso7816_user, &port->iso7816, sizeof(port->iso7816)))
+ return -EFAULT;
+
+ return 0;
+}
+
/*
* Called via sys_ioctl. We can use spin_lock_irq() here.
*/
@@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
case TIOCSRS485:
ret = uart_set_rs485_config(uport, uarg);
break;
+
+ case TIOCSISO7816:
+ ret = uart_set_iso7816_config(state->uart_port, uarg);
+ break;
+
+ case TIOCGISO7816:
+ ret = uart_get_iso7816_config(state->uart_port, uarg);
+ break;
default:
if (uport->ops->ioctl)
ret = uport->ops->ioctl(uport, cmd, arg);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 06ea4eeb09ab..d6e7747ffd46 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -137,6 +137,8 @@ struct uart_port {
void (*handle_break)(struct uart_port *);
int (*rs485_config)(struct uart_port *,
struct serial_rs485 *rs485);
+ int (*iso7816_config)(struct uart_port *,
+ struct serial_iso7816 *iso7816);
unsigned int irq; /* irq number */
unsigned long irqflags; /* irq flags */
unsigned int uartclk; /* base uart clock */
@@ -253,6 +255,7 @@ struct uart_port {
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
struct serial_rs485 rs485;
+ struct serial_iso7816 iso7816;
void *private_data; /* generic platform data pointer */
};
diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
index 040651735662..0e5c79726c2d 100644
--- a/include/uapi/asm-generic/ioctls.h
+++ b/include/uapi/asm-generic/ioctls.h
@@ -66,6 +66,8 @@
#ifndef TIOCSRS485
#define TIOCSRS485 0x542F
#endif
+#define TIOCGISO7816 0x5430
+#define TIOCSISO7816 0x5431
#define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of pty-mux device) */
#define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
#define TIOCGDEV _IOR('T', 0x32, unsigned int) /* Get primary device node of /dev/console */
diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
index 3fdd0dee8b41..93eb3c496ff1 100644
--- a/include/uapi/linux/serial.h
+++ b/include/uapi/linux/serial.h
@@ -132,4 +132,21 @@ struct serial_rs485 {
are a royal PITA .. */
};
+/*
+ * Serial interface for controlling ISO7816 settings on chips with suitable
+ * support. Set with TIOCSISO7816 and get with TIOCGISO7816 if supported by
+ * your platform.
+ */
+struct serial_iso7816 {
+ __u32 flags; /* ISO7816 feature flags */
+#define SER_ISO7816_ENABLED (1 << 0)
+#define SER_ISO7816_T_PARAM (0x0f << 4)
+#define SER_ISO7816_T(t) (((t) & 0x0f) << 4)
+ __u32 tg;
+ __u32 sc_fi;
+ __u32 sc_di;
+ __u32 clk;
+ __u32 reserved[5];
+};
+
#endif /* _UAPI_LINUX_SERIAL_H */
--
2.12.2
From: Nicolas Ferre <[email protected]>
In atmel_shutdown() we call atmel_stop_rx() and atmel_stop_tx() functions.
Prevent the rx restart that is implemented in RS485 or ISO7816 modes when
calling atmel_stop_tx() by using the atomic information tasklet_shutdown
that is already in place for this purpose.
Signed-off-by: Nicolas Ferre <[email protected]>
---
drivers/tty/serial/atmel_serial.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 0118b219f3a8..e4f877e1f3c6 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -617,7 +617,9 @@ static void atmel_stop_tx(struct uart_port *port)
if (((port->rs485.flags & SER_RS485_ENABLED) &&
!(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
port->iso7816.flags & SER_ISO7816_ENABLED)
- atmel_start_rx(port);
+ if (!atomic_read(&atmel_port->tasklet_shutdown))
+ atmel_start_rx(port);
+
}
/*
--
2.12.2
Hi Nicolas,
I love your patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.18-rc4 next-20180711]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ludovic-Desroches/add-ISO7816-support/20180712-052207
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sparc64
All errors (new ones prefixed by >>):
drivers/tty/serial/serial_core.c: In function 'uart_ioctl':
drivers/tty/serial/serial_core.c:1430:7: error: 'TIOCSISO7816' undeclared (first use in this function); did you mean 'TIOCSRS485'?
case TIOCSISO7816:
^~~~~~~~~~~~
TIOCSRS485
drivers/tty/serial/serial_core.c:1430:7: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/tty/serial/serial_core.c:1434:7: error: 'TIOCGISO7816' undeclared (first use in this function); did you mean 'TIOCSISO7816'?
case TIOCGISO7816:
^~~~~~~~~~~~
TIOCSISO7816
vim +1434 drivers/tty/serial/serial_core.c
1344
1345 /*
1346 * Called via sys_ioctl. We can use spin_lock_irq() here.
1347 */
1348 static int
1349 uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
1350 {
1351 struct uart_state *state = tty->driver_data;
1352 struct tty_port *port = &state->port;
1353 struct uart_port *uport;
1354 void __user *uarg = (void __user *)arg;
1355 int ret = -ENOIOCTLCMD;
1356
1357
1358 /*
1359 * These ioctls don't rely on the hardware to be present.
1360 */
1361 switch (cmd) {
1362 case TIOCGSERIAL:
1363 ret = uart_get_info_user(port, uarg);
1364 break;
1365
1366 case TIOCSSERIAL:
1367 down_write(&tty->termios_rwsem);
1368 ret = uart_set_info_user(tty, state, uarg);
1369 up_write(&tty->termios_rwsem);
1370 break;
1371
1372 case TIOCSERCONFIG:
1373 down_write(&tty->termios_rwsem);
1374 ret = uart_do_autoconfig(tty, state);
1375 up_write(&tty->termios_rwsem);
1376 break;
1377
1378 case TIOCSERGWILD: /* obsolete */
1379 case TIOCSERSWILD: /* obsolete */
1380 ret = 0;
1381 break;
1382 }
1383
1384 if (ret != -ENOIOCTLCMD)
1385 goto out;
1386
1387 if (tty_io_error(tty)) {
1388 ret = -EIO;
1389 goto out;
1390 }
1391
1392 /*
1393 * The following should only be used when hardware is present.
1394 */
1395 switch (cmd) {
1396 case TIOCMIWAIT:
1397 ret = uart_wait_modem_status(state, arg);
1398 break;
1399 }
1400
1401 if (ret != -ENOIOCTLCMD)
1402 goto out;
1403
1404 mutex_lock(&port->mutex);
1405 uport = uart_port_check(state);
1406
1407 if (!uport || tty_io_error(tty)) {
1408 ret = -EIO;
1409 goto out_up;
1410 }
1411
1412 /*
1413 * All these rely on hardware being present and need to be
1414 * protected against the tty being hung up.
1415 */
1416
1417 switch (cmd) {
1418 case TIOCSERGETLSR: /* Get line status register */
1419 ret = uart_get_lsr_info(tty, state, uarg);
1420 break;
1421
1422 case TIOCGRS485:
1423 ret = uart_get_rs485_config(uport, uarg);
1424 break;
1425
1426 case TIOCSRS485:
1427 ret = uart_set_rs485_config(uport, uarg);
1428 break;
1429
> 1430 case TIOCSISO7816:
1431 ret = uart_set_iso7816_config(state->uart_port, uarg);
1432 break;
1433
> 1434 case TIOCGISO7816:
1435 ret = uart_get_iso7816_config(state->uart_port, uarg);
1436 break;
1437 default:
1438 if (uport->ops->ioctl)
1439 ret = uport->ops->ioctl(uport, cmd, arg);
1440 break;
1441 }
1442 out_up:
1443 mutex_unlock(&port->mutex);
1444 out:
1445 return ret;
1446 }
1447
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Nicolas,
I love your patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.18-rc4 next-20180711]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Ludovic-Desroches/add-ISO7816-support/20180712-052207
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa
All errors (new ones prefixed by >>):
drivers/tty/serial/serial_core.c: In function 'uart_ioctl':
drivers/tty/serial/serial_core.c:1430:7: error: 'TIOCSISO7816' undeclared (first use in this function); did you mean 'TIOCSRS485'?
case TIOCSISO7816:
^~~~~~~~~~~~
TIOCSRS485
drivers/tty/serial/serial_core.c:1430:7: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/tty/serial/serial_core.c:1434:7: error: 'TIOCGISO7816' undeclared (first use in this function); did you mean 'TIOCGRS485'?
case TIOCGISO7816:
^~~~~~~~~~~~
TIOCGRS485
vim +1434 drivers/tty/serial/serial_core.c
1344
1345 /*
1346 * Called via sys_ioctl. We can use spin_lock_irq() here.
1347 */
1348 static int
1349 uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
1350 {
1351 struct uart_state *state = tty->driver_data;
1352 struct tty_port *port = &state->port;
1353 struct uart_port *uport;
1354 void __user *uarg = (void __user *)arg;
1355 int ret = -ENOIOCTLCMD;
1356
1357
1358 /*
1359 * These ioctls don't rely on the hardware to be present.
1360 */
1361 switch (cmd) {
1362 case TIOCGSERIAL:
1363 ret = uart_get_info_user(port, uarg);
1364 break;
1365
1366 case TIOCSSERIAL:
1367 down_write(&tty->termios_rwsem);
1368 ret = uart_set_info_user(tty, state, uarg);
1369 up_write(&tty->termios_rwsem);
1370 break;
1371
1372 case TIOCSERCONFIG:
1373 down_write(&tty->termios_rwsem);
1374 ret = uart_do_autoconfig(tty, state);
1375 up_write(&tty->termios_rwsem);
1376 break;
1377
1378 case TIOCSERGWILD: /* obsolete */
1379 case TIOCSERSWILD: /* obsolete */
1380 ret = 0;
1381 break;
1382 }
1383
1384 if (ret != -ENOIOCTLCMD)
1385 goto out;
1386
1387 if (tty_io_error(tty)) {
1388 ret = -EIO;
1389 goto out;
1390 }
1391
1392 /*
1393 * The following should only be used when hardware is present.
1394 */
1395 switch (cmd) {
1396 case TIOCMIWAIT:
1397 ret = uart_wait_modem_status(state, arg);
1398 break;
1399 }
1400
1401 if (ret != -ENOIOCTLCMD)
1402 goto out;
1403
1404 mutex_lock(&port->mutex);
1405 uport = uart_port_check(state);
1406
1407 if (!uport || tty_io_error(tty)) {
1408 ret = -EIO;
1409 goto out_up;
1410 }
1411
1412 /*
1413 * All these rely on hardware being present and need to be
1414 * protected against the tty being hung up.
1415 */
1416
1417 switch (cmd) {
1418 case TIOCSERGETLSR: /* Get line status register */
1419 ret = uart_get_lsr_info(tty, state, uarg);
1420 break;
1421
1422 case TIOCGRS485:
1423 ret = uart_get_rs485_config(uport, uarg);
1424 break;
1425
1426 case TIOCSRS485:
1427 ret = uart_set_rs485_config(uport, uarg);
1428 break;
1429
> 1430 case TIOCSISO7816:
1431 ret = uart_set_iso7816_config(state->uart_port, uarg);
1432 break;
1433
> 1434 case TIOCGISO7816:
1435 ret = uart_get_iso7816_config(state->uart_port, uarg);
1436 break;
1437 default:
1438 if (uport->ops->ioctl)
1439 ret = uport->ops->ioctl(uport, cmd, arg);
1440 break;
1441 }
1442 out_up:
1443 mutex_unlock(&port->mutex);
1444 out:
1445 return ret;
1446 }
1447
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <[email protected]>
>
> Add the ISO7816 ioctl and associated accessors and data structure.
> Drivers can then use this common implementation to handle ISO7816.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> [[email protected]: squash and rebase, removal of gpios, checkpatch fixes]
> Signed-off-by: Ludovic Desroches <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++++++++++++++++++++
> include/linux/serial_core.h | 3 +++
> include/uapi/asm-generic/ioctls.h | 2 ++
> include/uapi/linux/serial.h | 17 ++++++++++++++
> 4 files changed, 71 insertions(+)
Note, kbuild found build issues with this, please fix up.
Also, here's some comments:
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 9c14a453f73c..c89ac59f6f8c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port *port,
> return 0;
> }
>
> +static int uart_get_iso7816_config(struct uart_port *port,
> + struct serial_iso7816 __user *iso7816)
> +{
> + unsigned long flags;
> + struct serial_iso7816 aux;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + aux = port->iso7816;
Don't you want to check to see if there is a function for iso7816 before
copying it? Otherwise it's just empty, right?
> + if (!port->iso7816_config)
> + return -ENOIOCTLCMD;
Why this error value?
> --- a/include/uapi/asm-generic/ioctls.h
> +++ b/include/uapi/asm-generic/ioctls.h
> @@ -66,6 +66,8 @@
> #ifndef TIOCSRS485
> #define TIOCSRS485 0x542F
> #endif
> +#define TIOCGISO7816 0x5430
> +#define TIOCSISO7816 0x5431
Where did you get these numbers from?
thanks,
greg k-h
On Wed, Jul 11, 2018 at 03:26:23PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <[email protected]>
>
> In atmel_shutdown() we call atmel_stop_rx() and atmel_stop_tx() functions.
> Prevent the rx restart that is implemented in RS485 or ISO7816 modes when
> calling atmel_stop_tx() by using the atomic information tasklet_shutdown
> that is already in place for this purpose.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> ---
> drivers/tty/serial/atmel_serial.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Did you send this patch twice?
confused,
greg k-h
On Wed, Jul 11, 2018 at 03:16:38PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <[email protected]>
>
> In atmel_shutdown() we call atmel_stop_rx() and atmel_stop_tx() functions.
> Prevent the rx restart that is implemented in RS485 or ISO7816 modes when
> calling atmel_stop_tx() by using the atomic information tasklet_shutdown
> that is already in place for this purpose.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
No signed-off-by from you?
> ---
> drivers/tty/serial/atmel_serial.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 0118b219f3a8..e4f877e1f3c6 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -617,7 +617,9 @@ static void atmel_stop_tx(struct uart_port *port)
> if (((port->rs485.flags & SER_RS485_ENABLED) &&
> !(port->rs485.flags & SER_RS485_RX_DURING_TX)) ||
> port->iso7816.flags & SER_ISO7816_ENABLED)
> - atmel_start_rx(port);
> + if (!atomic_read(&atmel_port->tasklet_shutdown))
> + atmel_start_rx(port);
Is this really ok? No locking needed? What happens if that value is
set right after this?
thanks,
greg k-h
On Wed, Jul 11, 2018 at 03:16:37PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <[email protected]>
>
> When mode is set in atmel_config_iso7816() we backup last RS232 mode
> for coming back to this mode if requested.
> Also allow setup of T=0 and T=1 parameter and basic support in set_termios
> function as well.
> Report NACK and ITER errors in irq handler.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> [[email protected]: rebase, add check on fidi ratio, checkpatch fixes]
> Signed-off-by: Ludovic Desroches <[email protected]>
> ---
> drivers/tty/serial/atmel_serial.c | 175 +++++++++++++++++++++++++++++++++++---
> drivers/tty/serial/atmel_serial.h | 3 +-
> 2 files changed, 167 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 8e4428725848..0118b219f3a8 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -34,6 +34,7 @@
> #include <linux/suspend.h>
> #include <linux/mm.h>
>
> +#include <asm/div64.h>
> #include <asm/io.h>
> #include <asm/ioctls.h>
>
> @@ -147,6 +148,8 @@ struct atmel_uart_port {
> struct circ_buf rx_ring;
>
> struct mctrl_gpios *gpios;
> + u32 backup_mode; /* MR saved during iso7816 operations */
> + u32 backup_brgr; /* BRGR saved during iso7816 operations */
> unsigned int tx_done_mask;
> u32 fifo_size;
> u32 rts_high;
> @@ -362,6 +365,136 @@ static int atmel_config_rs485(struct uart_port *port,
> return 0;
> }
>
> +static unsigned int atmel_calc_cd(struct uart_port *port,
> + struct serial_iso7816 *iso7816conf)
> +{
> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> + unsigned int cd;
> + u64 mck_rate;
> +
> + mck_rate = (u64)clk_get_rate(atmel_port->clk);
> + dev_dbg(port->dev, "ISO7816 mck_rate = %lld\n", mck_rate);
> + do_div(mck_rate, iso7816conf->clk);
> + cd = mck_rate;
> + dev_dbg(port->dev, "ISO7816 clk = %d. CD = %d\n", iso7816conf->clk, cd);
> + return cd;
> +}
> +
> +static unsigned int atmel_calc_fidi(struct uart_port *port,
> + struct serial_iso7816 *iso7816conf)
> +{
> + u64 fidi = 0;
> +
> + dev_dbg(port->dev, "ISO7816 fi(%d) / di(%d)\n",
> + iso7816conf->sc_fi, iso7816conf->sc_di);
Do you still need all of the debugging code? Shouldn't ftrace give you
all of this?
> + if (iso7816conf->sc_fi && iso7816conf->sc_di) {
> + fidi = (u64)iso7816conf->sc_fi;
> + do_div(fidi, iso7816conf->sc_di);
> + }
> + dev_dbg(port->dev, "ISO7816 fidi = 0x%x\n", (u32)fidi);
> + return (u32)fidi;
> +}
> +
> +/* Enable or disable the iso7816 support */
> +/* Called with interrupts disabled */
> +static int atmel_config_iso7816(struct uart_port *port,
> + struct serial_iso7816 *iso7816conf)
> +{
> + struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
> + unsigned int mode, t;
> + unsigned int cd, fidi;
> + int ret = 0;
> +
> + /* Disable RX and TX */
> + atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RXDIS | ATMEL_US_TXDIS);
> + /* Disable interrupts */
> + atmel_uart_writel(port, ATMEL_US_IDR, atmel_port->tx_done_mask);
> +
> + mode = atmel_uart_readl(port, ATMEL_US_MR);
> +
> + if (!(port->iso7816.flags & SER_ISO7816_ENABLED)) {
> + /* port not yet in iso7816 mode: store configuration */
> + atmel_port->backup_mode = mode;
> + atmel_port->backup_brgr = atmel_uart_readl(port, ATMEL_US_BRGR);
> + }
Are you really validating all of the fields in this structure for valid
values correctly? It feels like you are missing some here...
> @@ -1281,6 +1417,9 @@ atmel_handle_status(struct uart_port *port, unsigned int pending,
> wake_up_interruptible(&port->state->port.delta_msr_wait);
> }
> }
> +
> + if (pending & (ATMEL_US_NACK | ATMEL_US_ITERATION))
> + dev_err(port->dev, "ISO7816 ERROR (0x%08x)\n", pending);
What can someone do with this?
If nothing, then why check or tell the user about it? They will ignore
it :(
thanks,
greg k-h
On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <[email protected]>
>
> Add the ISO7816 ioctl and associated accessors and data structure.
> Drivers can then use this common implementation to handle ISO7816.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> [[email protected]: squash and rebase, removal of gpios, checkpatch fixes]
> Signed-off-by: Ludovic Desroches <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++++++++++++++++++++
> include/linux/serial_core.h | 3 +++
> include/uapi/asm-generic/ioctls.h | 2 ++
> include/uapi/linux/serial.h | 17 ++++++++++++++
> 4 files changed, 71 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 9c14a453f73c..c89ac59f6f8c 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port *port,
> return 0;
> }
>
> +static int uart_get_iso7816_config(struct uart_port *port,
> + struct serial_iso7816 __user *iso7816)
> +{
> + unsigned long flags;
> + struct serial_iso7816 aux;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + aux = port->iso7816;
> + spin_unlock_irqrestore(&port->lock, flags);
> +
> + if (copy_to_user(iso7816, &aux, sizeof(aux)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int uart_set_iso7816_config(struct uart_port *port,
> + struct serial_iso7816 __user *iso7816_user)
> +{
> + struct serial_iso7816 iso7816;
> + int ret;
> + unsigned long flags;
> +
> + if (!port->iso7816_config)
> + return -ENOIOCTLCMD;
> +
> + if (copy_from_user(&iso7816, iso7816_user, sizeof(*iso7816_user)))
> + return -EFAULT;
> +
> + spin_lock_irqsave(&port->lock, flags);
> + ret = port->iso7816_config(port, &iso7816);
> + spin_unlock_irqrestore(&port->lock, flags);
> + if (ret)
> + return ret;
> +
> + if (copy_to_user(iso7816_user, &port->iso7816, sizeof(port->iso7816)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> /*
> * Called via sys_ioctl. We can use spin_lock_irq() here.
> */
> @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> case TIOCSRS485:
> ret = uart_set_rs485_config(uport, uarg);
> break;
> +
> + case TIOCSISO7816:
> + ret = uart_set_iso7816_config(state->uart_port, uarg);
> + break;
> +
> + case TIOCGISO7816:
> + ret = uart_get_iso7816_config(state->uart_port, uarg);
> + break;
> default:
> if (uport->ops->ioctl)
> ret = uport->ops->ioctl(uport, cmd, arg);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 06ea4eeb09ab..d6e7747ffd46 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -137,6 +137,8 @@ struct uart_port {
> void (*handle_break)(struct uart_port *);
> int (*rs485_config)(struct uart_port *,
> struct serial_rs485 *rs485);
> + int (*iso7816_config)(struct uart_port *,
> + struct serial_iso7816 *iso7816);
> unsigned int irq; /* irq number */
> unsigned long irqflags; /* irq flags */
> unsigned int uartclk; /* base uart clock */
> @@ -253,6 +255,7 @@ struct uart_port {
> struct attribute_group *attr_group; /* port specific attributes */
> const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> struct serial_rs485 rs485;
> + struct serial_iso7816 iso7816;
> void *private_data; /* generic platform data pointer */
> };
>
> diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
> index 040651735662..0e5c79726c2d 100644
> --- a/include/uapi/asm-generic/ioctls.h
> +++ b/include/uapi/asm-generic/ioctls.h
> @@ -66,6 +66,8 @@
> #ifndef TIOCSRS485
> #define TIOCSRS485 0x542F
> #endif
> +#define TIOCGISO7816 0x5430
> +#define TIOCSISO7816 0x5431
> #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of pty-mux device) */
> #define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
> #define TIOCGDEV _IOR('T', 0x32, unsigned int) /* Get primary device node of /dev/console */
> diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> index 3fdd0dee8b41..93eb3c496ff1 100644
> --- a/include/uapi/linux/serial.h
> +++ b/include/uapi/linux/serial.h
> @@ -132,4 +132,21 @@ struct serial_rs485 {
> are a royal PITA .. */
> };
>
> +/*
> + * Serial interface for controlling ISO7816 settings on chips with suitable
> + * support. Set with TIOCSISO7816 and get with TIOCGISO7816 if supported by
> + * your platform.
> + */
> +struct serial_iso7816 {
> + __u32 flags; /* ISO7816 feature flags */
> +#define SER_ISO7816_ENABLED (1 << 0)
> +#define SER_ISO7816_T_PARAM (0x0f << 4)
> +#define SER_ISO7816_T(t) (((t) & 0x0f) << 4)
> + __u32 tg;
> + __u32 sc_fi;
> + __u32 sc_di;
> + __u32 clk;
> + __u32 reserved[5];
You need to verify that reserved[] is all set to 0, otherwise people
will put crud in there and you can never use it for anything in the
future.
thanks,
greg k-h
On Thu, Jul 12, 2018 at 04:58:27PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:26:23PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre <[email protected]>
> >
> > In atmel_shutdown() we call atmel_stop_rx() and atmel_stop_tx() functions.
> > Prevent the rx restart that is implemented in RS485 or ISO7816 modes when
> > calling atmel_stop_tx() by using the atomic information tasklet_shutdown
> > that is already in place for this purpose.
> >
> > Signed-off-by: Nicolas Ferre <[email protected]>
> > ---
> > drivers/tty/serial/atmel_serial.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Did you send this patch twice?
>
> confused,
Yes sorry, as I didn't receive patch 3/3, I resend it. At the end, it
was sent twice.
Regards
Ludovic
>
> greg k-h
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Jul 12, 2018 at 05:02:29PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre <[email protected]>
> >
> > Add the ISO7816 ioctl and associated accessors and data structure.
> > Drivers can then use this common implementation to handle ISO7816.
> >
> > Signed-off-by: Nicolas Ferre <[email protected]>
> > [[email protected]: squash and rebase, removal of gpios, checkpatch fixes]
> > Signed-off-by: Ludovic Desroches <[email protected]>
> > ---
> > drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++++++++++++++++++++
> > include/linux/serial_core.h | 3 +++
> > include/uapi/asm-generic/ioctls.h | 2 ++
> > include/uapi/linux/serial.h | 17 ++++++++++++++
> > 4 files changed, 71 insertions(+)
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 9c14a453f73c..c89ac59f6f8c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port *port,
> > return 0;
> > }
> >
> > +static int uart_get_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816)
> > +{
> > + unsigned long flags;
> > + struct serial_iso7816 aux;
> > +
> > + spin_lock_irqsave(&port->lock, flags);
> > + aux = port->iso7816;
> > + spin_unlock_irqrestore(&port->lock, flags);
> > +
> > + if (copy_to_user(iso7816, &aux, sizeof(aux)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > +static int uart_set_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816_user)
> > +{
> > + struct serial_iso7816 iso7816;
> > + int ret;
> > + unsigned long flags;
> > +
> > + if (!port->iso7816_config)
> > + return -ENOIOCTLCMD;
> > +
> > + if (copy_from_user(&iso7816, iso7816_user, sizeof(*iso7816_user)))
> > + return -EFAULT;
> > +
> > + spin_lock_irqsave(&port->lock, flags);
> > + ret = port->iso7816_config(port, &iso7816);
> > + spin_unlock_irqrestore(&port->lock, flags);
> > + if (ret)
> > + return ret;
> > +
> > + if (copy_to_user(iso7816_user, &port->iso7816, sizeof(port->iso7816)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
> > +
> > /*
> > * Called via sys_ioctl. We can use spin_lock_irq() here.
> > */
> > @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> > case TIOCSRS485:
> > ret = uart_set_rs485_config(uport, uarg);
> > break;
> > +
> > + case TIOCSISO7816:
> > + ret = uart_set_iso7816_config(state->uart_port, uarg);
> > + break;
> > +
> > + case TIOCGISO7816:
> > + ret = uart_get_iso7816_config(state->uart_port, uarg);
> > + break;
> > default:
> > if (uport->ops->ioctl)
> > ret = uport->ops->ioctl(uport, cmd, arg);
> > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > index 06ea4eeb09ab..d6e7747ffd46 100644
> > --- a/include/linux/serial_core.h
> > +++ b/include/linux/serial_core.h
> > @@ -137,6 +137,8 @@ struct uart_port {
> > void (*handle_break)(struct uart_port *);
> > int (*rs485_config)(struct uart_port *,
> > struct serial_rs485 *rs485);
> > + int (*iso7816_config)(struct uart_port *,
> > + struct serial_iso7816 *iso7816);
> > unsigned int irq; /* irq number */
> > unsigned long irqflags; /* irq flags */
> > unsigned int uartclk; /* base uart clock */
> > @@ -253,6 +255,7 @@ struct uart_port {
> > struct attribute_group *attr_group; /* port specific attributes */
> > const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> > struct serial_rs485 rs485;
> > + struct serial_iso7816 iso7816;
> > void *private_data; /* generic platform data pointer */
> > };
> >
> > diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
> > index 040651735662..0e5c79726c2d 100644
> > --- a/include/uapi/asm-generic/ioctls.h
> > +++ b/include/uapi/asm-generic/ioctls.h
> > @@ -66,6 +66,8 @@
> > #ifndef TIOCSRS485
> > #define TIOCSRS485 0x542F
> > #endif
> > +#define TIOCGISO7816 0x5430
> > +#define TIOCSISO7816 0x5431
> > #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of pty-mux device) */
> > #define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
> > #define TIOCGDEV _IOR('T', 0x32, unsigned int) /* Get primary device node of /dev/console */
> > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > index 3fdd0dee8b41..93eb3c496ff1 100644
> > --- a/include/uapi/linux/serial.h
> > +++ b/include/uapi/linux/serial.h
> > @@ -132,4 +132,21 @@ struct serial_rs485 {
> > are a royal PITA .. */
> > };
> >
> > +/*
> > + * Serial interface for controlling ISO7816 settings on chips with suitable
> > + * support. Set with TIOCSISO7816 and get with TIOCGISO7816 if supported by
> > + * your platform.
> > + */
> > +struct serial_iso7816 {
> > + __u32 flags; /* ISO7816 feature flags */
> > +#define SER_ISO7816_ENABLED (1 << 0)
> > +#define SER_ISO7816_T_PARAM (0x0f << 4)
> > +#define SER_ISO7816_T(t) (((t) & 0x0f) << 4)
> > + __u32 tg;
> > + __u32 sc_fi;
> > + __u32 sc_di;
> > + __u32 clk;
> > + __u32 reserved[5];
>
> You need to verify that reserved[] is all set to 0, otherwise people
> will put crud in there and you can never use it for anything in the
> future.
Should I just verify or force it to 0?
Regards
Ludovic
>
> thanks,
>
> greg k-h
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Jul 12, 2018 at 04:58:08PM +0200, Greg KH wrote:
> On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > From: Nicolas Ferre <[email protected]>
> >
> > Add the ISO7816 ioctl and associated accessors and data structure.
> > Drivers can then use this common implementation to handle ISO7816.
> >
> > Signed-off-by: Nicolas Ferre <[email protected]>
> > [[email protected]: squash and rebase, removal of gpios, checkpatch fixes]
> > Signed-off-by: Ludovic Desroches <[email protected]>
> > ---
> > drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++++++++++++++++++++
> > include/linux/serial_core.h | 3 +++
> > include/uapi/asm-generic/ioctls.h | 2 ++
> > include/uapi/linux/serial.h | 17 ++++++++++++++
> > 4 files changed, 71 insertions(+)
>
> Note, kbuild found build issues with this, please fix up.
>
> Also, here's some comments:
>
> >
> > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > index 9c14a453f73c..c89ac59f6f8c 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port *port,
> > return 0;
> > }
> >
> > +static int uart_get_iso7816_config(struct uart_port *port,
> > + struct serial_iso7816 __user *iso7816)
> > +{
> > + unsigned long flags;
> > + struct serial_iso7816 aux;
> > +
> > + spin_lock_irqsave(&port->lock, flags);
> > + aux = port->iso7816;
>
> Don't you want to check to see if there is a function for iso7816 before
> copying it? Otherwise it's just empty, right?
I'll add the check.
>
> > + if (!port->iso7816_config)
> > + return -ENOIOCTLCMD;
>
> Why this error value?
>
It was a mimic of RS485.
> > --- a/include/uapi/asm-generic/ioctls.h
> > +++ b/include/uapi/asm-generic/ioctls.h
> > @@ -66,6 +66,8 @@
> > #ifndef TIOCSRS485
> > #define TIOCSRS485 0x542F
> > #endif
> > +#define TIOCGISO7816 0x5430
> > +#define TIOCSISO7816 0x5431
>
> Where did you get these numbers from?
I will use the macros to create numbers. Any rules about nr choice?
Regards
Ludovic
>
> thanks,
>
> greg k-h
On Fri, Jul 13, 2018 at 09:56:48AM +0200, Ludovic Desroches wrote:
> On Thu, Jul 12, 2018 at 05:02:29PM +0200, Greg KH wrote:
> > On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote:
> > > From: Nicolas Ferre <[email protected]>
> > >
> > > Add the ISO7816 ioctl and associated accessors and data structure.
> > > Drivers can then use this common implementation to handle ISO7816.
> > >
> > > Signed-off-by: Nicolas Ferre <[email protected]>
> > > [[email protected]: squash and rebase, removal of gpios, checkpatch fixes]
> > > Signed-off-by: Ludovic Desroches <[email protected]>
> > > ---
> > > drivers/tty/serial/serial_core.c | 49 +++++++++++++++++++++++++++++++++++++++
> > > include/linux/serial_core.h | 3 +++
> > > include/uapi/asm-generic/ioctls.h | 2 ++
> > > include/uapi/linux/serial.h | 17 ++++++++++++++
> > > 4 files changed, 71 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > > index 9c14a453f73c..c89ac59f6f8c 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -1301,6 +1301,47 @@ static int uart_set_rs485_config(struct uart_port *port,
> > > return 0;
> > > }
> > >
> > > +static int uart_get_iso7816_config(struct uart_port *port,
> > > + struct serial_iso7816 __user *iso7816)
> > > +{
> > > + unsigned long flags;
> > > + struct serial_iso7816 aux;
> > > +
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + aux = port->iso7816;
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > +
> > > + if (copy_to_user(iso7816, &aux, sizeof(aux)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int uart_set_iso7816_config(struct uart_port *port,
> > > + struct serial_iso7816 __user *iso7816_user)
> > > +{
> > > + struct serial_iso7816 iso7816;
> > > + int ret;
> > > + unsigned long flags;
> > > +
> > > + if (!port->iso7816_config)
> > > + return -ENOIOCTLCMD;
> > > +
> > > + if (copy_from_user(&iso7816, iso7816_user, sizeof(*iso7816_user)))
> > > + return -EFAULT;
> > > +
> > > + spin_lock_irqsave(&port->lock, flags);
> > > + ret = port->iso7816_config(port, &iso7816);
> > > + spin_unlock_irqrestore(&port->lock, flags);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (copy_to_user(iso7816_user, &port->iso7816, sizeof(port->iso7816)))
> > > + return -EFAULT;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > /*
> > > * Called via sys_ioctl. We can use spin_lock_irq() here.
> > > */
> > > @@ -1385,6 +1426,14 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg)
> > > case TIOCSRS485:
> > > ret = uart_set_rs485_config(uport, uarg);
> > > break;
> > > +
> > > + case TIOCSISO7816:
> > > + ret = uart_set_iso7816_config(state->uart_port, uarg);
> > > + break;
> > > +
> > > + case TIOCGISO7816:
> > > + ret = uart_get_iso7816_config(state->uart_port, uarg);
> > > + break;
> > > default:
> > > if (uport->ops->ioctl)
> > > ret = uport->ops->ioctl(uport, cmd, arg);
> > > diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> > > index 06ea4eeb09ab..d6e7747ffd46 100644
> > > --- a/include/linux/serial_core.h
> > > +++ b/include/linux/serial_core.h
> > > @@ -137,6 +137,8 @@ struct uart_port {
> > > void (*handle_break)(struct uart_port *);
> > > int (*rs485_config)(struct uart_port *,
> > > struct serial_rs485 *rs485);
> > > + int (*iso7816_config)(struct uart_port *,
> > > + struct serial_iso7816 *iso7816);
> > > unsigned int irq; /* irq number */
> > > unsigned long irqflags; /* irq flags */
> > > unsigned int uartclk; /* base uart clock */
> > > @@ -253,6 +255,7 @@ struct uart_port {
> > > struct attribute_group *attr_group; /* port specific attributes */
> > > const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
> > > struct serial_rs485 rs485;
> > > + struct serial_iso7816 iso7816;
> > > void *private_data; /* generic platform data pointer */
> > > };
> > >
> > > diff --git a/include/uapi/asm-generic/ioctls.h b/include/uapi/asm-generic/ioctls.h
> > > index 040651735662..0e5c79726c2d 100644
> > > --- a/include/uapi/asm-generic/ioctls.h
> > > +++ b/include/uapi/asm-generic/ioctls.h
> > > @@ -66,6 +66,8 @@
> > > #ifndef TIOCSRS485
> > > #define TIOCSRS485 0x542F
> > > #endif
> > > +#define TIOCGISO7816 0x5430
> > > +#define TIOCSISO7816 0x5431
> > > #define TIOCGPTN _IOR('T', 0x30, unsigned int) /* Get Pty Number (of pty-mux device) */
> > > #define TIOCSPTLCK _IOW('T', 0x31, int) /* Lock/unlock Pty */
> > > #define TIOCGDEV _IOR('T', 0x32, unsigned int) /* Get primary device node of /dev/console */
> > > diff --git a/include/uapi/linux/serial.h b/include/uapi/linux/serial.h
> > > index 3fdd0dee8b41..93eb3c496ff1 100644
> > > --- a/include/uapi/linux/serial.h
> > > +++ b/include/uapi/linux/serial.h
> > > @@ -132,4 +132,21 @@ struct serial_rs485 {
> > > are a royal PITA .. */
> > > };
> > >
> > > +/*
> > > + * Serial interface for controlling ISO7816 settings on chips with suitable
> > > + * support. Set with TIOCSISO7816 and get with TIOCGISO7816 if supported by
> > > + * your platform.
> > > + */
> > > +struct serial_iso7816 {
> > > + __u32 flags; /* ISO7816 feature flags */
> > > +#define SER_ISO7816_ENABLED (1 << 0)
> > > +#define SER_ISO7816_T_PARAM (0x0f << 4)
> > > +#define SER_ISO7816_T(t) (((t) & 0x0f) << 4)
> > > + __u32 tg;
> > > + __u32 sc_fi;
> > > + __u32 sc_di;
> > > + __u32 clk;
> > > + __u32 reserved[5];
> >
> > You need to verify that reserved[] is all set to 0, otherwise people
> > will put crud in there and you can never use it for anything in the
> > future.
>
> Should I just verify or force it to 0?
Verify please, and then abort if they are not set to 0. Otherwise if
userspace puts odd stuff in there, and then later you decide to use it
as new fields, that old code will start to do odd things.
thanks,
greg k-h
> >
> > > + if (!port->iso7816_config)
> > > + return -ENOIOCTLCMD;
> >
> > Why this error value?
> >
>
> It was a mimic of RS485.
Which is what you want - it means the upper tty layer knows to offer the
ioctl to other places and then return appropriately.
Alan