* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: kbuild test robot @ 2018-07-12 0:10 UTC (permalink / raw)
Cc: kbuild-all, linux-serial, linux-arch, linux-arm-kernel, gregkh,
jslaby, arnd, richard.genoud, nicolas.ferre, alexandre.belloni,
linux-kernel, Ludovic Desroches
In-Reply-To: <20180711131638.12622-2-ludovic.desroches@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 4350 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54028 bytes --]
^ permalink raw reply
* [PATCH v6 0/3] serial: 8250_dw: add fractional divisor support
From: Jisheng Zhang @ 2018-07-12 1:55 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-kernel, linux-serial, linux-kernel
For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits.
patch1 introduces necessary hooks to 8250 core.
patch2 exports serial8250_do_set_divisor()
patch3 implements the fractional divisor support for Synopsys DW 8250.
Since v5:
- Add Andy's Reviewed-by tag to all patches.
Since v4:
- use tmp to save baud * 16 rather than calculate every time in
dw8250_get_divisor()
Since v3:
- simplify the dw8250_get_divisor() implementation again.
Since v2:
- rebase to tty-next branch, since I need one patch from Andy which
is in tty-next
- drop the patch "serial: 8250: let serial8250_get_divisor() get
uart_port * as param" since it's in tty-next now.
- add a new patch to export serial8250_do_set_divisor(), and reuse it
to complete dw8250_set_divisor().
- remove DW 8250 version check, since the DLF register always exists
and if fractional divisor isn't supported, the register read as 0
- add comments to explain how dw8250_get_divisor() get quot and frac.
- the frac calcuation is simplified with well implemented GENMASK
- Add Andy's Reviewed-by tag to patch1.
Since v1:
- add an extra patch to let serial8250_get_divisor() get uart_port *
as param
- take Andy's suggestions to "integrates hooks in the same way like
it's done for the rest of 8250 ones". Many thanks to Andy.
Jisheng Zhang (3):
serial: 8250: introduce get_divisor() and set_divisor() hook
serial: 8250: export serial8250_do_set_divisor()
serial: 8250_dw: add fractional divisor support
drivers/tty/serial/8250/8250_core.c | 4 +++
drivers/tty/serial/8250/8250_dw.c | 45 +++++++++++++++++++++++++++++
drivers/tty/serial/8250/8250_port.c | 30 +++++++++++++++----
include/linux/serial_8250.h | 3 ++
include/linux/serial_core.h | 7 +++++
5 files changed, 84 insertions(+), 5 deletions(-)
--
2.18.0
^ permalink raw reply
* [PATCH v6 1/3] serial: 8250: introduce get_divisor() and set_divisor() hook
From: Jisheng Zhang @ 2018-07-12 1:57 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-kernel, linux-serial, linux-kernel
In-Reply-To: <20180712095504.4d9583ce@xhacker.debian>
Add these two hooks so that they can be overridden with driver specific
implementations.
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/8250/8250_core.c | 4 ++++
drivers/tty/serial/8250/8250_port.c | 27 +++++++++++++++++++++++----
include/linux/serial_core.h | 7 +++++++
3 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 9342fc2ee7df..a0bb77290747 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1023,6 +1023,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
uart->port.get_mctrl = up->port.get_mctrl;
if (up->port.set_mctrl)
uart->port.set_mctrl = up->port.set_mctrl;
+ if (up->port.get_divisor)
+ uart->port.get_divisor = up->port.get_divisor;
+ if (up->port.set_divisor)
+ uart->port.set_divisor = up->port.set_divisor;
if (up->port.startup)
uart->port.startup = up->port.startup;
if (up->port.shutdown)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 709fe6b4265c..ce0dc17f18ee 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2498,9 +2498,9 @@ static unsigned int npcm_get_divisor(struct uart_8250_port *up,
return DIV_ROUND_CLOSEST(port->uartclk, 16 * baud + 2) - 2;
}
-static unsigned int serial8250_get_divisor(struct uart_port *port,
- unsigned int baud,
- unsigned int *frac)
+static unsigned int serial8250_do_get_divisor(struct uart_port *port,
+ unsigned int baud,
+ unsigned int *frac)
{
struct uart_8250_port *up = up_to_u8250p(port);
unsigned int quot;
@@ -2532,6 +2532,16 @@ static unsigned int serial8250_get_divisor(struct uart_port *port,
return quot;
}
+static unsigned int serial8250_get_divisor(struct uart_port *port,
+ unsigned int baud,
+ unsigned int *frac)
+{
+ if (port->get_divisor)
+ return port->get_divisor(port, baud, frac);
+
+ return serial8250_do_get_divisor(port, baud, frac);
+}
+
static unsigned char serial8250_compute_lcr(struct uart_8250_port *up,
tcflag_t c_cflag)
{
@@ -2570,7 +2580,7 @@ static unsigned char serial8250_compute_lcr(struct uart_8250_port *up,
return cval;
}
-static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
+static void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
unsigned int quot, unsigned int quot_frac)
{
struct uart_8250_port *up = up_to_u8250p(port);
@@ -2603,6 +2613,15 @@ static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
}
}
+static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
+ unsigned int quot, unsigned int quot_frac)
+{
+ if (port->set_divisor)
+ port->set_divisor(port, baud, quot, quot_frac);
+ else
+ serial8250_do_set_divisor(port, baud, quot, quot_frac);
+}
+
static unsigned int serial8250_get_baud_rate(struct uart_port *port,
struct ktermios *termios,
struct ktermios *old)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 06ea4eeb09ab..406edae44ca3 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -127,6 +127,13 @@ struct uart_port {
struct ktermios *);
unsigned int (*get_mctrl)(struct uart_port *);
void (*set_mctrl)(struct uart_port *, unsigned int);
+ unsigned int (*get_divisor)(struct uart_port *,
+ unsigned int baud,
+ unsigned int *frac);
+ void (*set_divisor)(struct uart_port *,
+ unsigned int baud,
+ unsigned int quot,
+ unsigned int quot_frac);
int (*startup)(struct uart_port *port);
void (*shutdown)(struct uart_port *port);
void (*throttle)(struct uart_port *port);
--
2.18.0
^ permalink raw reply related
* [PATCH v6 2/3] serial: 8250: export serial8250_do_set_divisor()
From: Jisheng Zhang @ 2018-07-12 1:58 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-serial, linux-arm-kernel, linux-kernel
In-Reply-To: <20180712095504.4d9583ce@xhacker.debian>
Some drivers could call serial8250_do_set_divisor() to complete its
own set_divisor routine. Export this symbol for code reusing.
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/8250/8250_port.c | 5 +++--
include/linux/serial_8250.h | 3 +++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ce0dc17f18ee..945f8dc2d50f 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -2580,8 +2580,8 @@ static unsigned char serial8250_compute_lcr(struct uart_8250_port *up,
return cval;
}
-static void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
- unsigned int quot, unsigned int quot_frac)
+void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
+ unsigned int quot, unsigned int quot_frac)
{
struct uart_8250_port *up = up_to_u8250p(port);
@@ -2612,6 +2612,7 @@ static void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
serial_port_out(port, 0x2, quot_frac);
}
}
+EXPORT_SYMBOL_GPL(serial8250_do_set_divisor);
static void serial8250_set_divisor(struct uart_port *port, unsigned int baud,
unsigned int quot, unsigned int quot_frac)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index 76b9db71e489..18e21427bce4 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -160,6 +160,9 @@ extern void serial8250_do_shutdown(struct uart_port *port);
extern void serial8250_do_pm(struct uart_port *port, unsigned int state,
unsigned int oldstate);
extern void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl);
+extern void serial8250_do_set_divisor(struct uart_port *port, unsigned int baud,
+ unsigned int quot,
+ unsigned int quot_frac);
extern int fsl8250_handle_irq(struct uart_port *port);
int serial8250_handle_irq(struct uart_port *port, unsigned int iir);
unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr);
--
2.18.0
^ permalink raw reply related
* [PATCH v6 3/3] serial: 8250_dw: add fractional divisor support
From: Jisheng Zhang @ 2018-07-12 1:59 UTC (permalink / raw)
To: Andy Shevchenko, Greg Kroah-Hartman, Jiri Slaby
Cc: linux-arm-kernel, linux-serial, linux-kernel
In-Reply-To: <20180712095504.4d9583ce@xhacker.debian>
For Synopsys DesignWare 8250 uart which version >= 4.00a, there's a
valid divisor latch fraction register. The fractional divisor width is
4bits ~ 6bits.
Now the preparation is done, it's easy to add the feature support.
This patch firstly tries to get the fractional divisor width during
probe, then setups dw specific get_divisor() and set_divisor() hook.
Signed-off-by: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/tty/serial/8250/8250_dw.c | 45 +++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index fa8a00e8c9c6..5a60c4814d62 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -31,6 +31,7 @@
/* Offsets for the DesignWare specific registers */
#define DW_UART_USR 0x1f /* UART Status Register */
+#define DW_UART_DLF 0xc0 /* Divisor Latch Fraction Register */
#define DW_UART_CPR 0xf4 /* Component Parameter Register */
#define DW_UART_UCV 0xf8 /* UART Component Version */
@@ -55,6 +56,7 @@
struct dw8250_data {
u8 usr_reg;
+ u8 dlf_size;
int line;
int msr_mask_on;
int msr_mask_off;
@@ -366,6 +368,37 @@ static bool dw8250_idma_filter(struct dma_chan *chan, void *param)
return param == chan->device->dev->parent;
}
+/*
+ * divisor = div(I) + div(F)
+ * "I" means integer, "F" means fractional
+ * quot = div(I) = clk / (16 * baud)
+ * frac = div(F) * 2^dlf_size
+ *
+ * let rem = clk % (16 * baud)
+ * we have: div(F) * (16 * baud) = rem
+ * so frac = 2^dlf_size * rem / (16 * baud) = (rem << dlf_size) / (16 * baud)
+ */
+static unsigned int dw8250_get_divisor(struct uart_port *p,
+ unsigned int baud,
+ unsigned int *frac)
+{
+ unsigned int quot, rem, base_baud = baud * 16;
+ struct dw8250_data *d = p->private_data;
+
+ quot = p->uartclk / base_baud;
+ rem = p->uartclk % base_baud;
+ *frac = DIV_ROUND_CLOSEST(rem << d->dlf_size, base_baud);
+
+ return quot;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+ unsigned int quot, unsigned int quot_frac)
+{
+ dw8250_writel_ext(p, DW_UART_DLF, quot_frac);
+ serial8250_do_set_divisor(p, baud, quot, quot_frac);
+}
+
static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
{
if (p->dev->of_node) {
@@ -426,6 +459,18 @@ static void dw8250_setup_port(struct uart_port *p)
dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
+ dw8250_writel_ext(p, DW_UART_DLF, ~0U);
+ reg = dw8250_readl_ext(p, DW_UART_DLF);
+ dw8250_writel_ext(p, DW_UART_DLF, 0);
+
+ if (reg) {
+ struct dw8250_data *d = p->private_data;
+
+ d->dlf_size = fls(reg);
+ p->get_divisor = dw8250_get_divisor;
+ p->set_divisor = dw8250_set_divisor;
+ }
+
reg = dw8250_readl_ext(p, DW_UART_CPR);
if (!reg)
return;
--
2.18.0
^ permalink raw reply related
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Andy Shevchenko @ 2018-07-12 9:03 UTC (permalink / raw)
To: Tycho Andersen
Cc: Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Linux Kernel Mailing List, Serge E . Hallyn
In-Reply-To: <20180711160744.32074-1-tycho@tycho.ws>
On Wed, Jul 11, 2018 at 7:07 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> Anyway, since the lock is not acquired, if uart_shutdown() is called, the
> last chunk of that function may release state->xmit.buf before its assigned
> to null, and cause the race above.
>
> To fix it, let's lock uport->lock when allocating/deallocating
> state->xmit.buf in addition to the per-port mutex.
> * use the result of uart_port_lock() in uart_shutdown() to avoid
> uninitialized warning
> * don't use the uart_port_lock/unlock macros in uart_port_startup,
> instead test against uport directly; the compiler can't seem to "see"
> through the macros/ref/unref calls to not warn about uninitialized
> flags. We don't need to do a ref here since we hold the per-port
> mutex anyway.
> + if (uport)
> + spin_lock_irqsave(&uport->lock, flags);
> + if (uport)
> + spin_unlock_irqrestore(&uport->lock, flags);
At some point it It was uart_port_lock()/uart_port_unlock(), and you
changed to simple spin lock. The macro also take reference to the
port. Do we aware about that here?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Tycho Andersen @ 2018-07-12 14:13 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, open list:SERIAL DRIVERS,
Linux Kernel Mailing List, Serge E . Hallyn
In-Reply-To: <CAHp75VfQe4p_z1uAoq87F6zqaCbqKG1ZgHeK55fjwkFGNtMK0A@mail.gmail.com>
Hi Andy,
On Thu, Jul 12, 2018 at 12:03:08PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 11, 2018 at 7:07 PM, Tycho Andersen <tycho@tycho.ws> wrote:
>
> > Anyway, since the lock is not acquired, if uart_shutdown() is called, the
> > last chunk of that function may release state->xmit.buf before its assigned
> > to null, and cause the race above.
> >
> > To fix it, let's lock uport->lock when allocating/deallocating
> > state->xmit.buf in addition to the per-port mutex.
>
> > * use the result of uart_port_lock() in uart_shutdown() to avoid
> > uninitialized warning
> > * don't use the uart_port_lock/unlock macros in uart_port_startup,
> > instead test against uport directly; the compiler can't seem to "see"
> > through the macros/ref/unref calls to not warn about uninitialized
> > flags. We don't need to do a ref here since we hold the per-port
> > mutex anyway.
>
> > + if (uport)
> > + spin_lock_irqsave(&uport->lock, flags);
>
> > + if (uport)
> > + spin_unlock_irqrestore(&uport->lock, flags);
>
> At some point it It was uart_port_lock()/uart_port_unlock(), and you
> changed to simple spin lock. The macro also take reference to the
> port. Do we aware about that here?
I don't think so, the commit message you quoted above says,
> We don't need to do a ref here since we hold the per-port mutex
> anyway.
Cheers,
Tycho
^ permalink raw reply
* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: Greg KH @ 2018-07-12 14:58 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180711131638.12622-2-ludovic.desroches@microchip.com>
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
^ permalink raw reply
* Re: [PATCH 3/3] tty/serial: atmel: manage shutdown in case of RS485 or ISO7816 mode
From: Greg KH @ 2018-07-12 14:58 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180711132623.13227-1-ludovic.desroches@microchip.com>
On Wed, Jul 11, 2018 at 03:26:23PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> 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 <nicolas.ferre@microchip.com>
> ---
> drivers/tty/serial/atmel_serial.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Did you send this patch twice?
confused,
greg k-h
^ permalink raw reply
* Re: [PATCH 3/3] tty/serial: atmel: manage shutdown in case of RS485 or ISO7816 mode
From: Greg KH @ 2018-07-12 14:59 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180711131638.12622-4-ludovic.desroches@microchip.com>
On Wed, Jul 11, 2018 at 03:16:38PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> 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 <nicolas.ferre@microchip.com>
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
^ permalink raw reply
* Re: [PATCH 2/3] tty/serial: atmel: add ISO7816 support
From: Greg KH @ 2018-07-12 15:01 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180711131638.12622-3-ludovic.desroches@microchip.com>
On Wed, Jul 11, 2018 at 03:16:37PM +0200, Ludovic Desroches wrote:
> From: Nicolas Ferre <nicolas.ferre@microchip.com>
>
> 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 <nicolas.ferre@microchip.com>
> [ludovic.desroches@microchip.com: rebase, add check on fidi ratio, checkpatch fixes]
> Signed-off-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> ---
> 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
^ permalink raw reply
* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: Greg KH @ 2018-07-12 15:02 UTC (permalink / raw)
To: Ludovic Desroches
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180711131638.12622-2-ludovic.desroches@microchip.com>
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
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Greg Kroah-Hartman @ 2018-07-12 15:04 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Jiri Slaby, linux-serial, linux-kernel, Serge E . Hallyn
In-Reply-To: <20180711160744.32074-1-tycho@tycho.ws>
On Wed, Jul 11, 2018 at 10:07:44AM -0600, Tycho Andersen wrote:
> + if (uport)
> + spin_lock_irqsave(&uport->lock, flags);
That's the same thing as just calling uart_port_lock(), why aren't you
doing that?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Greg Kroah-Hartman @ 2018-07-12 15:05 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Serge E. Hallyn, Jiri Slaby, linux-serial, linux-kernel
In-Reply-To: <20180711200056.GA13192@cisco.cisco.com>
On Wed, Jul 11, 2018 at 02:00:56PM -0600, Tycho Andersen wrote:
> On Wed, Jul 11, 2018 at 02:49:08PM -0500, Serge E. Hallyn wrote:
> > Quoting Tycho Andersen (tycho@tycho.ws):
> > > We have reports of the following crash:
> > >
> > > PID: 7 TASK: ffff88085c6d61c0 CPU: 1 COMMAND: "kworker/u25:0"
> > > #0 [ffff88085c6db710] machine_kexec at ffffffff81046239
> > > #1 [ffff88085c6db760] crash_kexec at ffffffff810fc248
> > > #2 [ffff88085c6db830] oops_end at ffffffff81008ae7
> > > #3 [ffff88085c6db860] no_context at ffffffff81050b8f
> > > #4 [ffff88085c6db8b0] __bad_area_nosemaphore at ffffffff81050d75
> > > #5 [ffff88085c6db900] bad_area_nosemaphore at ffffffff81050e83
> > > #6 [ffff88085c6db910] __do_page_fault at ffffffff8105132e
> > > #7 [ffff88085c6db9b0] do_page_fault at ffffffff8105152c
> > > #8 [ffff88085c6db9c0] page_fault at ffffffff81a3f122
> > > [exception RIP: uart_put_char+149]
> > > RIP: ffffffff814b67b5 RSP: ffff88085c6dba78 RFLAGS: 00010006
> > > RAX: 0000000000000292 RBX: ffffffff827c5120 RCX: 0000000000000081
> > > RDX: 0000000000000000 RSI: 000000000000005f RDI: ffffffff827c5120
> > > RBP: ffff88085c6dba98 R8: 000000000000012c R9: ffffffff822ea320
> > > R10: ffff88085fe4db04 R11: 0000000000000001 R12: ffff881059f9c000
> > > R13: 0000000000000001 R14: 000000000000005f R15: 0000000000000fba
> > > ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
> > > #9 [ffff88085c6dbaa0] tty_put_char at ffffffff81497544
> > > #10 [ffff88085c6dbac0] do_output_char at ffffffff8149c91c
> > > #11 [ffff88085c6dbae0] __process_echoes at ffffffff8149cb8b
> > > #12 [ffff88085c6dbb30] commit_echoes at ffffffff8149cdc2
> > > #13 [ffff88085c6dbb60] n_tty_receive_buf_fast at ffffffff8149e49b
> > > #14 [ffff88085c6dbbc0] __receive_buf at ffffffff8149ef5a
> > > #15 [ffff88085c6dbc20] n_tty_receive_buf_common at ffffffff8149f016
> > > #16 [ffff88085c6dbca0] n_tty_receive_buf2 at ffffffff8149f194
> > > #17 [ffff88085c6dbcb0] flush_to_ldisc at ffffffff814a238a
> > > #18 [ffff88085c6dbd50] process_one_work at ffffffff81090be2
> > > #19 [ffff88085c6dbe20] worker_thread at ffffffff81091b4d
> > > #20 [ffff88085c6dbeb0] kthread at ffffffff81096384
> > > #21 [ffff88085c6dbf50] ret_from_fork at ffffffff81a3d69f
> > >
> > > after slogging through some dissasembly:
> > >
> > > ffffffff814b6720 <uart_put_char>:
> > > ffffffff814b6720: 55 push %rbp
> > > ffffffff814b6721: 48 89 e5 mov %rsp,%rbp
> > > ffffffff814b6724: 48 83 ec 20 sub $0x20,%rsp
> > > ffffffff814b6728: 48 89 1c 24 mov %rbx,(%rsp)
> > > ffffffff814b672c: 4c 89 64 24 08 mov %r12,0x8(%rsp)
> > > ffffffff814b6731: 4c 89 6c 24 10 mov %r13,0x10(%rsp)
> > > ffffffff814b6736: 4c 89 74 24 18 mov %r14,0x18(%rsp)
> > > ffffffff814b673b: e8 b0 8e 58 00 callq ffffffff81a3f5f0 <mcount>
> > > ffffffff814b6740: 4c 8b a7 88 02 00 00 mov 0x288(%rdi),%r12
> > > ffffffff814b6747: 45 31 ed xor %r13d,%r13d
> > > ffffffff814b674a: 41 89 f6 mov %esi,%r14d
> > > ffffffff814b674d: 49 83 bc 24 70 01 00 cmpq $0x0,0x170(%r12)
> > > ffffffff814b6754: 00 00
> > > ffffffff814b6756: 49 8b 9c 24 80 01 00 mov 0x180(%r12),%rbx
> > > ffffffff814b675d: 00
> > > ffffffff814b675e: 74 2f je ffffffff814b678f <uart_put_char+0x6f>
> > > ffffffff814b6760: 48 89 df mov %rbx,%rdi
> > > ffffffff814b6763: e8 a8 67 58 00 callq ffffffff81a3cf10 <_raw_spin_lock_irqsave>
> > > ffffffff814b6768: 41 8b 8c 24 78 01 00 mov 0x178(%r12),%ecx
> > > ffffffff814b676f: 00
> > > ffffffff814b6770: 89 ca mov %ecx,%edx
> > > ffffffff814b6772: f7 d2 not %edx
> > > ffffffff814b6774: 41 03 94 24 7c 01 00 add 0x17c(%r12),%edx
> > > ffffffff814b677b: 00
> > > ffffffff814b677c: 81 e2 ff 0f 00 00 and $0xfff,%edx
> > > ffffffff814b6782: 75 23 jne ffffffff814b67a7 <uart_put_char+0x87>
> > > ffffffff814b6784: 48 89 c6 mov %rax,%rsi
> > > ffffffff814b6787: 48 89 df mov %rbx,%rdi
> > > ffffffff814b678a: e8 e1 64 58 00 callq ffffffff81a3cc70 <_raw_spin_unlock_irqrestore>
> > > ffffffff814b678f: 44 89 e8 mov %r13d,%eax
> > > ffffffff814b6792: 48 8b 1c 24 mov (%rsp),%rbx
> > > ffffffff814b6796: 4c 8b 64 24 08 mov 0x8(%rsp),%r12
> > > ffffffff814b679b: 4c 8b 6c 24 10 mov 0x10(%rsp),%r13
> > > ffffffff814b67a0: 4c 8b 74 24 18 mov 0x18(%rsp),%r14
> > > ffffffff814b67a5: c9 leaveq
> > > ffffffff814b67a6: c3 retq
> > > ffffffff814b67a7: 49 8b 94 24 70 01 00 mov 0x170(%r12),%rdx
> > > ffffffff814b67ae: 00
> > > ffffffff814b67af: 48 63 c9 movslq %ecx,%rcx
> > > ffffffff814b67b2: 41 b5 01 mov $0x1,%r13b
> > > ffffffff814b67b5: 44 88 34 0a mov %r14b,(%rdx,%rcx,1)
> > > ffffffff814b67b9: 41 8b 94 24 78 01 00 mov 0x178(%r12),%edx
> > > ffffffff814b67c0: 00
> > > ffffffff814b67c1: 83 c2 01 add $0x1,%edx
> > > ffffffff814b67c4: 81 e2 ff 0f 00 00 and $0xfff,%edx
> > > ffffffff814b67ca: 41 89 94 24 78 01 00 mov %edx,0x178(%r12)
> > > ffffffff814b67d1: 00
> > > ffffffff814b67d2: eb b0 jmp ffffffff814b6784 <uart_put_char+0x64>
> > > ffffffff814b67d4: 66 66 66 2e 0f 1f 84 data32 data32 nopw %cs:0x0(%rax,%rax,1)
> > > ffffffff814b67db: 00 00 00 00 00
> > >
> > > for our build, this is crashing at:
> > >
> > > circ->buf[circ->head] = c;
> > >
> > > Looking in uart_port_startup(), it seems that circ->buf (state->xmit.buf)
> > > protected by the "per-port mutex", which based on uart_port_check() is
> > > state->port.mutex. Indeed, the lock acquired in uart_put_char() is
> > > uport->lock, i.e. not the same lock.
> > >
> > > Anyway, since the lock is not acquired, if uart_shutdown() is called, the
> > > last chunk of that function may release state->xmit.buf before its assigned
> > > to null, and cause the race above.
> > >
> > > To fix it, let's lock uport->lock when allocating/deallocating
> > > state->xmit.buf in addition to the per-port mutex.
> > >
> > > v2: switch to locking uport->lock on allocation/deallocation instead of
> > > locking the per-port mutex in uart_put_char. Note that since
> > > uport->lock is a spin lock, we have to switch the allocation to
> > > GFP_ATOMIC.
> > > v3: move the allocation outside the lock, so we can switch back to
> > > GFP_KERNEL
> > > v4: * switch to positive condition of state->xmit.buf in
> > > uart_port_startup()
> > > * declare `flags` on its own line
> > > * use the result of uart_port_lock() in uart_shutdown() to avoid
> > > uninitialized warning
> > > * don't use the uart_port_lock/unlock macros in uart_port_startup,
> > > instead test against uport directly; the compiler can't seem to "see"
> > > through the macros/ref/unref calls to not warn about uninitialized
> > > flags. We don't need to do a ref here since we hold the per-port
> > > mutex anyway.
> > >
> > > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> >
> > One question: would it be worth doing:
> >
> > > ---
> > > drivers/tty/serial/serial_core.c | 19 ++++++++++++++-----
> > > 1 file changed, 14 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> > > index 9c14a453f73c..406e8382d3de 100644
> > > --- a/drivers/tty/serial/serial_core.c
> > > +++ b/drivers/tty/serial/serial_core.c
> > > @@ -182,6 +182,7 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> > > {
> > > struct uart_port *uport = uart_port_check(state);
> > > unsigned long page;
> > > + unsigned long flags;
> > > int retval = 0;
> > >
> > > if (uport->type == PORT_UNKNOWN)
> > > @@ -196,15 +197,20 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
> > > * Initialise and allocate the transmit and temporary
> > > * buffer.
> > > */
> > > - if (!state->xmit.buf) {
> > > - /* This is protected by the per port mutex */
> > > - page = get_zeroed_page(GFP_KERNEL);
> > > - if (!page)
> > > - return -ENOMEM;
> > > + page = get_zeroed_page(GFP_KERNEL);
> >
> > ignoring the alloc failure here. (leave a comment for worried
> > future reviewers) Then,
> >
> > > + if (!page)
> > > + return -ENOMEM;
> > >
> > > + if (uport)
> > > + spin_lock_irqsave(&uport->lock, flags);
> > > + if (state->xmit.buf) {
> > > + free_page(page);
> > > + } else {
> >
> > here if page is NULL (unlock and) return ENOMEM.
> >
> > Since it looks like this fn is called on every device open, that
> > would seem to minimize getting lots of needless ENOMEMs.
> >
> > Another alternative is to just wait to do the alloc until we
> > get here, and drop the spinlock here if we need to alloc. Then
> > retake the lock, re-check state->xmit.buf; If that is now not
> > null then free_page(page), or if it is still NULL and page alloc
> > failed, then return ENOEMEM). That is uglier code, but is
> > probably the best behavior.
> >
> > Still the original patch fixes the real bug so I'm fine with it
> > as well.
>
> Sure, I'm happy to implement whichever of these we think is best.
> Greg?
Let's fix this issue first please, then I'll be glad to review other
changes based on micro optimizations :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Tycho Andersen @ 2018-07-12 15:08 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Serge E . Hallyn
In-Reply-To: <20180712150438.GF22502@kroah.com>
On Thu, Jul 12, 2018 at 05:04:38PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 11, 2018 at 10:07:44AM -0600, Tycho Andersen wrote:
> > + if (uport)
> > + spin_lock_irqsave(&uport->lock, flags);
>
> That's the same thing as just calling uart_port_lock(), why aren't you
> doing that?
Because the compiler can't seem to "see" through the macros/ref calls,
and I get the warning I mentioned here if I use them:
https://lkml.org/lkml/2018/7/6/840
Tycho
^ permalink raw reply
* Re: [PATCH 3/3] tty/serial: atmel: manage shutdown in case of RS485 or ISO7816 mode
From: Ludovic Desroches @ 2018-07-12 15:23 UTC (permalink / raw)
To: Greg KH
Cc: linux-arch, alexandre.belloni, arnd, richard.genoud, linux-kernel,
linux-serial, jslaby, linux-arm-kernel
In-Reply-To: <20180712145827.GB22502@kroah.com>
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 <nicolas.ferre@microchip.com>
> >
> > 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 <nicolas.ferre@microchip.com>
> > ---
> > 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
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Greg Kroah-Hartman @ 2018-07-12 15:40 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Jiri Slaby, linux-serial, linux-kernel, Serge E . Hallyn
In-Reply-To: <20180712150822.GC13192@cisco.cisco.com>
On Thu, Jul 12, 2018 at 09:08:22AM -0600, Tycho Andersen wrote:
> On Thu, Jul 12, 2018 at 05:04:38PM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jul 11, 2018 at 10:07:44AM -0600, Tycho Andersen wrote:
> > > + if (uport)
> > > + spin_lock_irqsave(&uport->lock, flags);
> >
> > That's the same thing as just calling uart_port_lock(), why aren't you
> > doing that?
>
> Because the compiler can't seem to "see" through the macros/ref calls,
> and I get the warning I mentioned here if I use them:
>
> https://lkml.org/lkml/2018/7/6/840
What horrible version of gcc are you using that give you that? Don't
open-code things just because of a broken compiler.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Tycho Andersen @ 2018-07-12 18:18 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Serge E . Hallyn
In-Reply-To: <20180712154015.GC13114@kroah.com>
On Thu, Jul 12, 2018 at 05:40:15PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 12, 2018 at 09:08:22AM -0600, Tycho Andersen wrote:
> > On Thu, Jul 12, 2018 at 05:04:38PM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 11, 2018 at 10:07:44AM -0600, Tycho Andersen wrote:
> > > > + if (uport)
> > > > + spin_lock_irqsave(&uport->lock, flags);
> > >
> > > That's the same thing as just calling uart_port_lock(), why aren't you
> > > doing that?
> >
> > Because the compiler can't seem to "see" through the macros/ref calls,
> > and I get the warning I mentioned here if I use them:
> >
> > https://lkml.org/lkml/2018/7/6/840
>
> What horrible version of gcc are you using that give you that? Don't
> open-code things just because of a broken compiler.
I've tried with both 7.3.0 and 5.4.0. I think the reason we see this
here but not elsewhere in the file is because there's an actual
function call (free_page()) in the critical section.
If we move that out, something like the below patch, it all works for
me.
Tycho
>From 532e82ceec67142b71c60ce74ce08c5339195a94 Mon Sep 17 00:00:00 2001
From: Tycho Andersen <tycho@tycho.ws>
Date: Mon, 4 Jun 2018 09:55:09 -0600
Subject: [PATCH] uart: fix race between uart_put_char() and uart_shutdown()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
We have reports of the following crash:
PID: 7 TASK: ffff88085c6d61c0 CPU: 1 COMMAND: "kworker/u25:0"
#0 [ffff88085c6db710] machine_kexec at ffffffff81046239
#1 [ffff88085c6db760] crash_kexec at ffffffff810fc248
#2 [ffff88085c6db830] oops_end at ffffffff81008ae7
#3 [ffff88085c6db860] no_context at ffffffff81050b8f
#4 [ffff88085c6db8b0] __bad_area_nosemaphore at ffffffff81050d75
#5 [ffff88085c6db900] bad_area_nosemaphore at ffffffff81050e83
#6 [ffff88085c6db910] __do_page_fault at ffffffff8105132e
#7 [ffff88085c6db9b0] do_page_fault at ffffffff8105152c
#8 [ffff88085c6db9c0] page_fault at ffffffff81a3f122
[exception RIP: uart_put_char+149]
RIP: ffffffff814b67b5 RSP: ffff88085c6dba78 RFLAGS: 00010006
RAX: 0000000000000292 RBX: ffffffff827c5120 RCX: 0000000000000081
RDX: 0000000000000000 RSI: 000000000000005f RDI: ffffffff827c5120
RBP: ffff88085c6dba98 R8: 000000000000012c R9: ffffffff822ea320
R10: ffff88085fe4db04 R11: 0000000000000001 R12: ffff881059f9c000
R13: 0000000000000001 R14: 000000000000005f R15: 0000000000000fba
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#9 [ffff88085c6dbaa0] tty_put_char at ffffffff81497544
#10 [ffff88085c6dbac0] do_output_char at ffffffff8149c91c
#11 [ffff88085c6dbae0] __process_echoes at ffffffff8149cb8b
#12 [ffff88085c6dbb30] commit_echoes at ffffffff8149cdc2
#13 [ffff88085c6dbb60] n_tty_receive_buf_fast at ffffffff8149e49b
#14 [ffff88085c6dbbc0] __receive_buf at ffffffff8149ef5a
#15 [ffff88085c6dbc20] n_tty_receive_buf_common at ffffffff8149f016
#16 [ffff88085c6dbca0] n_tty_receive_buf2 at ffffffff8149f194
#17 [ffff88085c6dbcb0] flush_to_ldisc at ffffffff814a238a
#18 [ffff88085c6dbd50] process_one_work at ffffffff81090be2
#19 [ffff88085c6dbe20] worker_thread at ffffffff81091b4d
#20 [ffff88085c6dbeb0] kthread at ffffffff81096384
#21 [ffff88085c6dbf50] ret_from_fork at ffffffff81a3d69f
after slogging through some dissasembly:
ffffffff814b6720 <uart_put_char>:
ffffffff814b6720: 55 push %rbp
ffffffff814b6721: 48 89 e5 mov %rsp,%rbp
ffffffff814b6724: 48 83 ec 20 sub $0x20,%rsp
ffffffff814b6728: 48 89 1c 24 mov %rbx,(%rsp)
ffffffff814b672c: 4c 89 64 24 08 mov %r12,0x8(%rsp)
ffffffff814b6731: 4c 89 6c 24 10 mov %r13,0x10(%rsp)
ffffffff814b6736: 4c 89 74 24 18 mov %r14,0x18(%rsp)
ffffffff814b673b: e8 b0 8e 58 00 callq ffffffff81a3f5f0 <mcount>
ffffffff814b6740: 4c 8b a7 88 02 00 00 mov 0x288(%rdi),%r12
ffffffff814b6747: 45 31 ed xor %r13d,%r13d
ffffffff814b674a: 41 89 f6 mov %esi,%r14d
ffffffff814b674d: 49 83 bc 24 70 01 00 cmpq $0x0,0x170(%r12)
ffffffff814b6754: 00 00
ffffffff814b6756: 49 8b 9c 24 80 01 00 mov 0x180(%r12),%rbx
ffffffff814b675d: 00
ffffffff814b675e: 74 2f je ffffffff814b678f <uart_put_char+0x6f>
ffffffff814b6760: 48 89 df mov %rbx,%rdi
ffffffff814b6763: e8 a8 67 58 00 callq ffffffff81a3cf10 <_raw_spin_lock_irqsave>
ffffffff814b6768: 41 8b 8c 24 78 01 00 mov 0x178(%r12),%ecx
ffffffff814b676f: 00
ffffffff814b6770: 89 ca mov %ecx,%edx
ffffffff814b6772: f7 d2 not %edx
ffffffff814b6774: 41 03 94 24 7c 01 00 add 0x17c(%r12),%edx
ffffffff814b677b: 00
ffffffff814b677c: 81 e2 ff 0f 00 00 and $0xfff,%edx
ffffffff814b6782: 75 23 jne ffffffff814b67a7 <uart_put_char+0x87>
ffffffff814b6784: 48 89 c6 mov %rax,%rsi
ffffffff814b6787: 48 89 df mov %rbx,%rdi
ffffffff814b678a: e8 e1 64 58 00 callq ffffffff81a3cc70 <_raw_spin_unlock_irqrestore>
ffffffff814b678f: 44 89 e8 mov %r13d,%eax
ffffffff814b6792: 48 8b 1c 24 mov (%rsp),%rbx
ffffffff814b6796: 4c 8b 64 24 08 mov 0x8(%rsp),%r12
ffffffff814b679b: 4c 8b 6c 24 10 mov 0x10(%rsp),%r13
ffffffff814b67a0: 4c 8b 74 24 18 mov 0x18(%rsp),%r14
ffffffff814b67a5: c9 leaveq
ffffffff814b67a6: c3 retq
ffffffff814b67a7: 49 8b 94 24 70 01 00 mov 0x170(%r12),%rdx
ffffffff814b67ae: 00
ffffffff814b67af: 48 63 c9 movslq %ecx,%rcx
ffffffff814b67b2: 41 b5 01 mov $0x1,%r13b
ffffffff814b67b5: 44 88 34 0a mov %r14b,(%rdx,%rcx,1)
ffffffff814b67b9: 41 8b 94 24 78 01 00 mov 0x178(%r12),%edx
ffffffff814b67c0: 00
ffffffff814b67c1: 83 c2 01 add $0x1,%edx
ffffffff814b67c4: 81 e2 ff 0f 00 00 and $0xfff,%edx
ffffffff814b67ca: 41 89 94 24 78 01 00 mov %edx,0x178(%r12)
ffffffff814b67d1: 00
ffffffff814b67d2: eb b0 jmp ffffffff814b6784 <uart_put_char+0x64>
ffffffff814b67d4: 66 66 66 2e 0f 1f 84 data32 data32 nopw %cs:0x0(%rax,%rax,1)
ffffffff814b67db: 00 00 00 00 00
for our build, this is crashing at:
circ->buf[circ->head] = c;
Looking in uart_port_startup(), it seems that circ->buf (state->xmit.buf)
protected by the "per-port mutex", which based on uart_port_check() is
state->port.mutex. Indeed, the lock acquired in uart_put_char() is
uport->lock, i.e. not the same lock.
Anyway, since the lock is not acquired, if uart_shutdown() is called, the
last chunk of that function may release state->xmit.buf before its assigned
to null, and cause the race above.
To fix it, let's lock uport->lock when allocating/deallocating
state->xmit.buf in addition to the per-port mutex.
v2: switch to locking uport->lock on allocation/deallocation instead of
locking the per-port mutex in uart_put_char. Note that since
uport->lock is a spin lock, we have to switch the allocation to
GFP_ATOMIC.
v3: move the allocation outside the lock, so we can switch back to
GFP_KERNEL
v4: * switch to positive condition of state->xmit.buf in
uart_port_startup()
* declare `flags` on its own line
* use the result of uart_port_lock() in uart_shutdown() to avoid
uninitialized warning
* don't use the uart_port_lock/unlock macros in uart_port_startup,
instead test against uport directly; the compiler can't seem to "see"
through the macros/ref/unref calls to not warn about uninitialized
flags. We don't need to do a ref here since we hold the per-port
mutex anyway.
v5: use uart_port_lock/unlock, but free the page outside the critical
section if it is unused
Signed-off-by: Tycho Andersen <tycho@tycho.ws>
---
drivers/tty/serial/serial_core.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c14a453f73c..91b292bbda91 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -182,6 +182,8 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
{
struct uart_port *uport = uart_port_check(state);
unsigned long page;
+ unsigned long flags;
+ bool page_used = false;
int retval = 0;
if (uport->type == PORT_UNKNOWN)
@@ -196,15 +198,20 @@ static int uart_port_startup(struct tty_struct *tty, struct uart_state *state,
* Initialise and allocate the transmit and temporary
* buffer.
*/
- if (!state->xmit.buf) {
- /* This is protected by the per port mutex */
- page = get_zeroed_page(GFP_KERNEL);
- if (!page)
- return -ENOMEM;
+ page = get_zeroed_page(GFP_KERNEL);
+ if (!page)
+ return -ENOMEM;
+ uport = uart_port_lock(state, flags);
+ if (!state->xmit.buf) {
state->xmit.buf = (unsigned char *) page;
uart_circ_clear(&state->xmit);
+ page_used = true;
}
+ uart_port_unlock(uport, flags);
+
+ if (!page_used)
+ free_page(page);
retval = uport->ops->startup(uport);
if (retval == 0) {
@@ -263,6 +270,7 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
{
struct uart_port *uport = uart_port_check(state);
struct tty_port *port = &state->port;
+ unsigned long flags;
/*
* Set the TTY IO error marker
@@ -295,10 +303,12 @@ static void uart_shutdown(struct tty_struct *tty, struct uart_state *state)
/*
* Free the transmit buffer page.
*/
+ uport = uart_port_lock(state, flags);
if (state->xmit.buf) {
free_page((unsigned long)state->xmit.buf);
state->xmit.buf = NULL;
}
+ uart_port_unlock(uport, flags);
}
/**
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Greg Kroah-Hartman @ 2018-07-12 18:25 UTC (permalink / raw)
To: Tycho Andersen; +Cc: Jiri Slaby, linux-serial, linux-kernel, Serge E . Hallyn
In-Reply-To: <20180712181846.GD13192@cisco.cisco.com>
On Thu, Jul 12, 2018 at 12:18:46PM -0600, Tycho Andersen wrote:
> On Thu, Jul 12, 2018 at 05:40:15PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 12, 2018 at 09:08:22AM -0600, Tycho Andersen wrote:
> > > On Thu, Jul 12, 2018 at 05:04:38PM +0200, Greg Kroah-Hartman wrote:
> > > > On Wed, Jul 11, 2018 at 10:07:44AM -0600, Tycho Andersen wrote:
> > > > > + if (uport)
> > > > > + spin_lock_irqsave(&uport->lock, flags);
> > > >
> > > > That's the same thing as just calling uart_port_lock(), why aren't you
> > > > doing that?
> > >
> > > Because the compiler can't seem to "see" through the macros/ref calls,
> > > and I get the warning I mentioned here if I use them:
> > >
> > > https://lkml.org/lkml/2018/7/6/840
> >
> > What horrible version of gcc are you using that give you that? Don't
> > open-code things just because of a broken compiler.
>
> I've tried with both 7.3.0 and 5.4.0. I think the reason we see this
> here but not elsewhere in the file is because there's an actual
> function call (free_page()) in the critical section.
>
> If we move that out, something like the below patch, it all works for
> me.
Ick. Which version of this series had the problem? Let me test it out
here...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v4] uart: fix race between uart_put_char() and uart_shutdown()
From: Tycho Andersen @ 2018-07-12 18:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, linux-serial, linux-kernel, Serge E . Hallyn
In-Reply-To: <20180712182545.GA30099@kroah.com>
On Thu, Jul 12, 2018 at 08:25:45PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 12, 2018 at 12:18:46PM -0600, Tycho Andersen wrote:
> > On Thu, Jul 12, 2018 at 05:40:15PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jul 12, 2018 at 09:08:22AM -0600, Tycho Andersen wrote:
> > > > On Thu, Jul 12, 2018 at 05:04:38PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Wed, Jul 11, 2018 at 10:07:44AM -0600, Tycho Andersen wrote:
> > > > > > + if (uport)
> > > > > > + spin_lock_irqsave(&uport->lock, flags);
> > > > >
> > > > > That's the same thing as just calling uart_port_lock(), why aren't you
> > > > > doing that?
> > > >
> > > > Because the compiler can't seem to "see" through the macros/ref calls,
> > > > and I get the warning I mentioned here if I use them:
> > > >
> > > > https://lkml.org/lkml/2018/7/6/840
> > >
> > > What horrible version of gcc are you using that give you that? Don't
> > > open-code things just because of a broken compiler.
> >
> > I've tried with both 7.3.0 and 5.4.0. I think the reason we see this
> > here but not elsewhere in the file is because there's an actual
> > function call (free_page()) in the critical section.
> >
> > If we move that out, something like the below patch, it all works for
> > me.
>
> Ick. Which version of this series had the problem? Let me test it out
> here...
v3, if you remove the initialization of flags from both functions you
should see it.
Tycho
^ permalink raw reply
* Re: [PATCH v10 0/6] Driver for at91 usart in spi mode
From: Lee Jones @ 2018-07-13 7:37 UTC (permalink / raw)
To: Nicolas Ferre
Cc: Radu Pirea, broonie, alexandre.belloni, richard.genoud, robh+dt,
mark.rutland, gregkh, linux-spi, linux-arm-kernel, linux-kernel,
devicetree, linux-serial
In-Reply-To: <aecba39e-deba-c64b-9542-a446e074e95e@microchip.com>
On Wed, 11 Jul 2018, Nicolas Ferre wrote:
> On 25/06/2018 at 19:22, Radu Pirea wrote:
> > Hello,
> >
> > This is the second version of driver. I added a mfd driver which by
> > default probes atmel_serial driver and if in dt is specified to probe
> > the spi driver, then the spi-at91-usart driver will be probed. The
> > compatible for atmel_serial is now the compatible for at91-usart mfd
> > driver and compatilbe for atmel_serial driver was changed in order to
> > keep the bindings for serial as they are.
> >
> > Changes in v10:
> > -fixed kbuild test robot warning
> >
> > Changes in v9:
> > - minor changes
> > - rebased on top of broonie/for-4.19
> >
> > Changes in v8:
> > - fixed passing an empty mfd cell if "atmel,usart-mode" value is invalid
> >
> > Changes in v7:
> > - synced up SPDIX license with module license
> > - numbering of usart modes starts from 0 insteand of 1
> >
> > Changes in v6:
> > - removed unused compatible strings from serial and spi drivers
> >
> > Changes in v5:
> > - fixed usage of stdout-path property with atmel_serial driver
> >
> > Changes in v4:
> > - modified the spi driver to use cs gpio support form spi subsystem
> > - fixed dma transfers for serial driver
> > - squashed binding for spi and serial and moved them to mfd/atmel-usart.txt
> >
> > Changes in v3:
> > - fixed spi slaves probing
> >
> > Changes in v2:
> > - added at91-usart mfd driver
> > - modified spi-at91-usart driver to work as mfd driver child
> > - modified atmel_serial driver to work as mfd driver child
> >
> > Changes in v1:
> > - added spi-at91-usart driver
> >
> >
> > Radu Pirea (6):
> > MAINTAINERS: add at91 usart mfd driver
> > dt-bindings: add binding for atmel-usart in SPI mode
> > mfd: at91-usart: added mfd driver for usart
> > MAINTAINERS: add at91 usart spi driver
> > spi: at91-usart: add driver for at91-usart as spi
> > tty/serial: atmel: change the driver to work under at91-usart mfd
>
> For the record or if needed by the MAINTAINERS changes, you can add my:
> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
> for the whole series.
>
> Once the remarks by Mark are addressed, you can certainly re-send the series
> with all tags collected for Lee to take it, if he is okay with this process,
> of course...
That's fine. No problem.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: Ludovic Desroches @ 2018-07-13 7:56 UTC (permalink / raw)
To: Greg KH
Cc: linux-arch, alexandre.belloni, arnd, richard.genoud, linux-kernel,
linux-serial, jslaby, linux-arm-kernel
In-Reply-To: <20180712150229.GE22502@kroah.com>
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
^ permalink raw reply
* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: Ludovic Desroches @ 2018-07-13 8:01 UTC (permalink / raw)
To: Greg KH
Cc: linux-serial, linux-arch, linux-arm-kernel, jslaby, arnd,
richard.genoud, nicolas.ferre, alexandre.belloni, linux-kernel
In-Reply-To: <20180712145808.GA22502@kroah.com>
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
^ permalink raw reply
* Re: [PATCH 1/3] tty/serial_core: add ISO7816 infrastructure
From: Greg KH @ 2018-07-13 8:16 UTC (permalink / raw)
To: linux-arch, alexandre.belloni, arnd, richard.genoud, linux-kernel,
linux-serial, jslaby, linux-arm-kernel
In-Reply-To: <20180713075648.ya2er5eip6uldl75@M43218.corp.atmel.com>
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
^ permalink raw reply
* [PATCH] tty: serial: jsm: remove redundant pointer ch
From: Colin King @ 2018-07-13 9:05 UTC (permalink / raw)
To: Guilherme G . Piccoli, Greg Kroah-Hartman, Jiri Slaby,
linux-serial
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Pointer ch is being assigned but is never used hence it is redundant
and can be removed.
Cleans up clang warning:
warning: variable 'ch' set but not used [-Wunused-but-set-variable]
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/tty/serial/jsm/jsm_tty.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/jsm/jsm_tty.c b/drivers/tty/serial/jsm/jsm_tty.c
index b6bd6e15e07b..689774c073ca 100644
--- a/drivers/tty/serial/jsm/jsm_tty.c
+++ b/drivers/tty/serial/jsm/jsm_tty.c
@@ -430,7 +430,6 @@ int jsm_uart_port_init(struct jsm_board *brd)
{
int i, rc;
unsigned int line;
- struct jsm_channel *ch;
if (!brd)
return -ENXIO;
@@ -444,7 +443,7 @@ int jsm_uart_port_init(struct jsm_board *brd)
brd->nasync = brd->maxports;
/* Set up channel variables */
- for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
+ for (i = 0; i < brd->nasync; i++) {
if (!brd->channels[i])
continue;
--
2.17.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox