| Message ID | 20180711131638.12622-2-ludovic.desroches@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <nicolas.ferre@microchip.com> > > 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 <nicolas.ferre@microchip.com> > [ludovic.desroches@microchip.com: squash and rebase, removal of gpios, checkpatch fixes] > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > 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:16:36PM +0200, Ludovic Desroches wrote: > From: Nicolas Ferre <nicolas.ferre@microchip.com> > > 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 <nicolas.ferre@microchip.com> > [ludovic.desroches@microchip.com: squash and rebase, removal of gpios, checkpatch fixes] > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > 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 05:02:29PM +0200, Greg KH wrote: > On Wed, Jul 11, 2018 at 03:16:36PM +0200, Ludovic Desroches wrote: > > From: Nicolas Ferre <nicolas.ferre@microchip.com> > > > > 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 <nicolas.ferre@microchip.com> > > [ludovic.desroches@microchip.com: squash and rebase, removal of gpios, checkpatch fixes] > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > --- > > 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 > linux-arm-kernel@lists.infradead.org > 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 <nicolas.ferre@microchip.com> > > > > 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 <nicolas.ferre@microchip.com> > > [ludovic.desroches@microchip.com: squash and rebase, removal of gpios, checkpatch fixes] > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > --- > > 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 <nicolas.ferre@microchip.com> > > > > > > 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 <nicolas.ferre@microchip.com> > > > [ludovic.desroches@microchip.com: squash and rebase, removal of gpios, checkpatch fixes] > > > Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > > --- > > > 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
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 */