* Re: [PATCH] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Geert Uytterhoeven @ 2018-05-08 7:29 UTC (permalink / raw)
To: Daniel Wagner
Cc: Sebastian Andrzej Siewior, Linux Kernel Mailing List,
linux-rt-users, open list:SERIAL DRIVERS, Greg Kroah-Hartman,
Linux-sh list, Daniel Wagner, Shinya Kuribayashi
In-Reply-To: <66cd1363-9f62-e2f5-2271-1fa78509f2cd@monom.org>
Hi Daniel,
On Tue, May 8, 2018 at 9:23 AM, Daniel Wagner <wagi@monom.org> wrote:
> On 05/07/2018 02:47 PM, Sebastian Andrzej Siewior wrote:
>> On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote:
>>>> --- a/drivers/tty/serial/sh-sci.c
>>>> +++ b/drivers/tty/serial/sh-sci.c
>>>> @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console
>>>> *co, const char *s,
>>>> unsigned long flags;
>>>> int locked = 1;
>>>>
>>>> - local_irq_save(flags);
>>>
>>>
>>> Hence the below now runs with local interrupts enabled.
>>>
>>> For checking port->sysrq or oops_in_progress that probably isn't an
>>> issue.
>>> If oops_in_progress is set, you have other problems, and the race
>>> condition
>>> between checking the flag and calling spin_lock{,_irqsave}() existed
>>> before,
>>> and is hard to avoid.
>>
>> while oops_in_progress is an issue of its own, the port->sysrq isn't
>> avoided by by local_irq_save(). On SMP systems you can still receive a
>> `break' signal on the UART and have a `printk()' issued on another CPU.
>>
>>> For actual console printing, I think you want to keep interrupts
>>> disabled.
>>
>> why? They should be disabled as part of getting the lock and not for any
>> other reason.
>>
>>>> if (port->sysrq)
>>>> locked = 0;
>>>> else if (oops_in_progress)
>>>> - locked = spin_trylock(&port->lock);
>>>> + locked = spin_trylock_irqsave(&port->lock, flags);
>>>> else
>>>> - spin_lock(&port->lock);
>>>> + spin_lock_irqsave(&port->lock, flags);
>>>
>>>
>>> Add
>>>
>>> if (!locked
>>> local_irq_save(flags)
>>>
>>> here?
>>
>>
>> So for oops_in_progress you get here with interrupts disabled. And if
>> not, I don't see the point in disabling the interrupts without any kind
>> of locking.
>
>
> So I understand, the initial version of this patch was correct.
>
> @Geert if you don't object I'll send a v3 (v1 ported to mainline).
Please go ahead, thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Daniel Wagner @ 2018-05-08 7:23 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, Geert Uytterhoeven
Cc: Linux Kernel Mailing List, linux-rt-users,
open list:SERIAL DRIVERS, Greg Kroah-Hartman, Linux-sh list,
Daniel Wagner, Shinya Kuribayashi
In-Reply-To: <20180507124704.s4qlrcc3leoky4r7@linutronix.de>
On 05/07/2018 02:47 PM, Sebastian Andrzej Siewior wrote:
> On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote:
>>> --- a/drivers/tty/serial/sh-sci.c
>>> +++ b/drivers/tty/serial/sh-sci.c
>>> @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s,
>>> unsigned long flags;
>>> int locked = 1;
>>>
>>> - local_irq_save(flags);
>>
>> Hence the below now runs with local interrupts enabled.
>>
>> For checking port->sysrq or oops_in_progress that probably isn't an issue.
>> If oops_in_progress is set, you have other problems, and the race condition
>> between checking the flag and calling spin_lock{,_irqsave}() existed before,
>> and is hard to avoid.
>
> while oops_in_progress is an issue of its own, the port->sysrq isn't
> avoided by by local_irq_save(). On SMP systems you can still receive a
> `break' signal on the UART and have a `printk()' issued on another CPU.
>
>> For actual console printing, I think you want to keep interrupts disabled.
>
> why? They should be disabled as part of getting the lock and not for any
> other reason.
>
>>> if (port->sysrq)
>>> locked = 0;
>>> else if (oops_in_progress)
>>> - locked = spin_trylock(&port->lock);
>>> + locked = spin_trylock_irqsave(&port->lock, flags);
>>> else
>>> - spin_lock(&port->lock);
>>> + spin_lock_irqsave(&port->lock, flags);
>>
>> Add
>>
>> if (!locked
>> local_irq_save(flags)
>>
>> here?
>
> So for oops_in_progress you get here with interrupts disabled. And if
> not, I don't see the point in disabling the interrupts without any kind
> of locking.
So I understand, the initial version of this patch was correct.
@Geert if you don't object I'll send a v3 (v1 ported to mainline).
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Daniel Wagner @ 2018-05-08 7:18 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: linux-kernel, Geert Uytterhoeven, linux-rt-users, linux-serial,
linux-sh, gregkh, Daniel Wagner
In-Reply-To: <20180507125146.75crpaj2scav7mql@linutronix.de>
Hi Sebastian,
On 05/07/2018 02:51 PM, Sebastian Andrzej Siewior wrote:
> On 2018-05-04 18:30:41 [+0200], Daniel Wagner wrote:
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s,
>> unsigned long flags;
>> int locked = 1;
>>
>> - local_irq_save(flags);
>> #if defined(SUPPORT_SYSRQ)
>> - if (port->sysrq)
>> + if (port->sysrq) {
>> locked = 0;
>> - else
>> + local_irq_save(flags);
>
> how is this helping? You should see a splat after a sysrq request.
You are right, I didn't really think this through.
Should 'echo t > /proc/sysrq' trigger the splat? At least I was so naive
that think it would be enough.
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v3 2/3] platform: move the early platform device support to arch/sh
From: Geert Uytterhoeven @ 2018-05-08 6:45 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
Stephen Boyd, Arnd Bergmann, Greg Kroah-Hartman, Mark Rutland,
Yoshinori Sato, Rich Felker, Andy Shevchenko, Marc Zyngier,
Rafael J . Wysocki, Peter Rosin, Jiri Slaby, Thomas Gleixner,
Daniel Lezcano, Magnus Damm, Linux ARM
In-Reply-To: <20180504132731.14574-3-brgl@bgdev.pl>
Hi Bartosz,
On Fri, May 4, 2018 at 3:27 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> SuperH is the only user of the current implementation of early platform
> device support. We want to introduce a more robust approach to early
> probing. As the first step - move all the current early platform code
> to arch/sh.
>
> In order not to export internal drivers/base functions to arch code for
> this temporary solution - copy the two needed routines for driver
> matching from drivers/base/platform.c to arch/sh/drivers/platform_early.c.
>
> Also: call early_platform_cleanup() from subsys_initcall() so that it's
> called after all early devices are probed.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> drivers/clocksource/sh_cmt.c | 7 +
> drivers/clocksource/sh_mtu2.c | 7 +
> drivers/clocksource/sh_tmu.c | 8 +
> drivers/tty/serial/sh-sci.c | 7 +-
> include/linux/platform_device.h | 64 +----
The parts used on contemporary ARM/ARM64 Renesas SoCs look fine to me, so
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Uwe Kleine-König @ 2018-05-08 6:43 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180507213610.17330-3-sebastian.reichel@collabora.co.uk>
On Mon, May 07, 2018 at 11:36:10PM +0200, Sebastian Reichel wrote:
> This properly unmaps DMA SG on device shutdown.
>
> Reported-by: Nandor Han <nandor.han@ge.com>
> Suggested-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
> drivers/tty/serial/imx.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 3ca767b1162a..6c53e74244ec 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
> u32 ucr1, ucr2;
>
> if (sport->dma_is_enabled) {
> - sport->dma_is_rxing = 0;
> - sport->dma_is_txing = 0;
> dmaengine_terminate_sync(sport->dma_chan_tx);
> + if (sport->dma_is_txing) {
> + dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
> + sport->dma_tx_nents, DMA_TO_DEVICE);
> + sport->dma_is_txing = 0;
> + }
did you find this because the kernel crashed or consumed more and more
memory, or is this "only" a finding of reading the source code? If the
former it would be great to point out in the commit log, if the latter,
I wonder if this is a real problem that warrants a stable backport.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()
From: Uwe Kleine-König @ 2018-05-08 6:40 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, Fabio Estevam,
Shawn Guo, linux-kernel
In-Reply-To: <20180507213610.17330-2-sebastian.reichel@collabora.co.uk>
Hello,
On Mon, May 07, 2018 at 11:36:09PM +0200, Sebastian Reichel wrote:
> Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
> move it to imx_uart_shutdown(), which is the only user of the DMA
> disabling function. This should not change the driver's behaviour,
> but improves readability. After this change imx_uart_disable_dma()
> does the reverse thing of imx_uart_enable_dma().
>
> Suggested-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
> drivers/tty/serial/imx.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index c2fc6bef7a6f..3ca767b1162a 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
>
> static void imx_uart_disable_dma(struct imx_port *sport)
> {
> - u32 ucr1, ucr2;
> + u32 ucr1;
>
> /* clear UCR1 */
> ucr1 = imx_uart_readl(sport, UCR1);
> ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
> imx_uart_writel(sport, ucr1, UCR1);
>
> - /* clear UCR2 */
> - ucr2 = imx_uart_readl(sport, UCR2);
> - ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> - imx_uart_writel(sport, ucr2, UCR2);
> -
> imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
>
> sport->dma_is_enabled = 0;
> @@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
>
> spin_lock_irqsave(&sport->port.lock, flags);
> ucr2 = imx_uart_readl(sport, UCR2);
> - ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
> + ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
> imx_uart_writel(sport, ucr2, UCR2);
> spin_unlock_irqrestore(&sport->port.lock, flags);
While this doesn't change behaviour (which is of course good and
intended here) I wonder if changing RTS is right here.
According to Documentation/serial/driver it is not.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v3 1/3] clocksource: timer-ti-dm: remove the early platform driver registration
From: Daniel Lezcano @ 2018-05-08 1:40 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
Stephen Boyd, Arnd Bergmann, Greg Kroah-Hartman, Mark Rutland,
Yoshinori Sato, Rich Felker, Andy Shevchenko, Marc Zyngier,
Rafael J . Wysocki, Peter Rosin, Jiri Slaby, Thomas Gleixner,
Geert Uytterhoeven, Magnus Damm, linux-arm-kernel, linux-kernel,
linux-serial
In-Reply-To: <20180504132731.14574-2-brgl@bgdev.pl>
On Fri, May 04, 2018 at 03:27:29PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This driver is no longer used as an early platform driver. Remove the
> registration macro.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* [PATCH 2/2] serial: imx: dma_unmap_sg buffers on shutdown
From: Sebastian Reichel @ 2018-05-07 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
Sebastian Reichel
In-Reply-To: <20180507213610.17330-1-sebastian.reichel@collabora.co.uk>
This properly unmaps DMA SG on device shutdown.
Reported-by: Nandor Han <nandor.han@ge.com>
Suggested-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/tty/serial/imx.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 3ca767b1162a..6c53e74244ec 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1425,10 +1425,18 @@ static void imx_uart_shutdown(struct uart_port *port)
u32 ucr1, ucr2;
if (sport->dma_is_enabled) {
- sport->dma_is_rxing = 0;
- sport->dma_is_txing = 0;
dmaengine_terminate_sync(sport->dma_chan_tx);
+ if (sport->dma_is_txing) {
+ dma_unmap_sg(sport->port.dev, &sport->tx_sgl[0],
+ sport->dma_tx_nents, DMA_TO_DEVICE);
+ sport->dma_is_txing = 0;
+ }
dmaengine_terminate_sync(sport->dma_chan_rx);
+ if (sport->dma_is_rxing) {
+ dma_unmap_sg(sport->port.dev, &sport->rx_sgl,
+ 1, DMA_FROM_DEVICE);
+ sport->dma_is_rxing = 0;
+ }
spin_lock_irqsave(&sport->port.lock, flags);
imx_uart_stop_tx(port);
--
2.17.0
^ permalink raw reply related
* [PATCH 1/2] serial: imx: cleanup imx_uart_disable_dma()
From: Sebastian Reichel @ 2018-05-07 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
Sebastian Reichel
In-Reply-To: <20180507213610.17330-1-sebastian.reichel@collabora.co.uk>
Remove unrelated CTSC/CTS disabling from imx_uart_disable_dma() and
move it to imx_uart_shutdown(), which is the only user of the DMA
disabling function. This should not change the driver's behaviour,
but improves readability. After this change imx_uart_disable_dma()
does the reverse thing of imx_uart_enable_dma().
Suggested-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
drivers/tty/serial/imx.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index c2fc6bef7a6f..3ca767b1162a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1291,18 +1291,13 @@ static void imx_uart_enable_dma(struct imx_port *sport)
static void imx_uart_disable_dma(struct imx_port *sport)
{
- u32 ucr1, ucr2;
+ u32 ucr1;
/* clear UCR1 */
ucr1 = imx_uart_readl(sport, UCR1);
ucr1 &= ~(UCR1_RXDMAEN | UCR1_TXDMAEN | UCR1_ATDMAEN);
imx_uart_writel(sport, ucr1, UCR1);
- /* clear UCR2 */
- ucr2 = imx_uart_readl(sport, UCR2);
- ucr2 &= ~(UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
- imx_uart_writel(sport, ucr2, UCR2);
-
imx_uart_setup_ufcr(sport, TXTL_DEFAULT, RXTL_DEFAULT);
sport->dma_is_enabled = 0;
@@ -1447,7 +1442,7 @@ static void imx_uart_shutdown(struct uart_port *port)
spin_lock_irqsave(&sport->port.lock, flags);
ucr2 = imx_uart_readl(sport, UCR2);
- ucr2 &= ~(UCR2_TXEN | UCR2_ATEN);
+ ucr2 &= ~(UCR2_TXEN | UCR2_CTSC | UCR2_CTS | UCR2_ATEN);
imx_uart_writel(sport, ucr2, UCR2);
spin_unlock_irqrestore(&sport->port.lock, flags);
--
2.17.0
^ permalink raw reply related
* [PATCH 0/2] serial: imx: cleanup shutdown
From: Sebastian Reichel @ 2018-05-07 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, linux-serial
Cc: Uwe Kleine-König, Fabio Estevam, Shawn Guo, linux-kernel,
Sebastian Reichel
Hi,
Here is a simple cleanup patch for the imx serial driver. The second
one fixes a missing dma resource free. I tested both patches on imx53
based GE PPD device.
-- Sebastian
Sebastian Reichel (2):
serial: imx: cleanup imx_uart_disable_dma()
serial: imx: dma_unmap_sg buffers on shutdown
drivers/tty/serial/imx.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
--
2.17.0
^ permalink raw reply
* Re: [PATCH v3 2/3] platform: move the early platform device support to arch/sh
From: Arnd Bergmann @ 2018-05-07 21:13 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
Stephen Boyd, Greg Kroah-Hartman, Mark Rutland, Yoshinori Sato,
Rich Felker, Andy Shevchenko, Marc Zyngier, Rafael J . Wysocki,
Peter Rosin, Jiri Slaby, Thomas Gleixner, Daniel Lezcano,
Geert Uytterhoeven, Magnus Damm, Linux ARM
In-Reply-To: <20180504132731.14574-3-brgl@bgdev.pl>
On Fri, May 4, 2018 at 9:27 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> SuperH is the only user of the current implementation of early platform
> device support. We want to introduce a more robust approach to early
> probing. As the first step - move all the current early platform code
> to arch/sh.
>
> In order not to export internal drivers/base functions to arch code for
> this temporary solution - copy the two needed routines for driver
> matching from drivers/base/platform.c to arch/sh/drivers/platform_early.c.
>
> Also: call early_platform_cleanup() from subsys_initcall() so that it's
> called after all early devices are probed.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
I had not gotten around to look at this in enough detail so far, but
looking now, it seems all good.
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* [PATCH v3] tty/serial: atmel: use port->name as name in request_irq()
From: Sebastian Andrzej Siewior @ 2018-05-07 17:11 UTC (permalink / raw)
To: Rob Herring
Cc: Alexandre Belloni, Peter Hurley, Richard Genoud,
Greg Kroah-Hartman, open list:SERIAL DRIVERS, Jiri Slaby,
Thomas Gleixner,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_JsqJ+84OFyN07XWsb0hSHxtxL_V6gSfO+iMSOU1bceztr3Q@mail.gmail.com>
I was puzzled while looking at /proc/interrupts and random things showed
up between reboots. This occurred more often but I realised it later. The
"correct" output should be:
|38: 11861 atmel-aic5 2 Level ttyS0
but I saw sometimes
|38: 6426 atmel-aic5 2 Level tty1
and accounted it wrongly as correct. This is use after free and the
former example randomly got the "old" pointer which pointed to the same
content. With SLAB_FREELIST_RANDOM and HARDENED I even got
|38: 7067 atmel-aic5 2 Level E=Started User Manager for UID 0
or other nonsense.
As it turns out the tty, pointer that is accessed in atmel_startup(), is
freed() before atmel_shutdown(). It seems to happen quite often that the
tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
don't do anything special - just a systemd boot :)
Use dev_name(&pdev->dev) as the IRQ name for request_irq(). This exists
as long as the driver is loaded so no use-after-free here.
Cc: stable@vger.kernel.org
Fixes: 761ed4a94582 ("tty: serial_core: convert uart_close to use tty_port_close")
Acked-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v2…v3: - Add Acked-by
- replace port->name -> dev_name(&pdev->dev) to ease work for
backporters (suggested by Rob)
v1…v2: add Fixes after bisect.
drivers/tty/serial/atmel_serial.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index e287fe8f10fc..55b3eff148b1 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1757,7 +1757,6 @@ static int atmel_startup(struct uart_port *port)
{
struct platform_device *pdev = to_platform_device(port->dev);
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
- struct tty_struct *tty = port->state->port.tty;
int retval;
/*
@@ -1772,8 +1771,8 @@ static int atmel_startup(struct uart_port *port)
* Allocate the IRQ
*/
retval = request_irq(port->irq, atmel_interrupt,
- IRQF_SHARED | IRQF_COND_SUSPEND,
- tty ? tty->name : "atmel_serial", port);
+ IRQF_SHARED | IRQF_COND_SUSPEND,
+ dev_name(&pdev->dev), port);
if (retval) {
dev_err(port->dev, "atmel_startup - Can't get irq\n");
return retval;
--
2.17.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v2] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Sebastian Andrzej Siewior @ 2018-05-07 12:51 UTC (permalink / raw)
To: Daniel Wagner
Cc: linux-kernel, Geert Uytterhoeven, linux-rt-users, linux-serial,
linux-sh, gregkh, Daniel Wagner
In-Reply-To: <20180504163041.28726-1-wagi@monom.org>
On 2018-05-04 18:30:41 [+0200], Daniel Wagner wrote:
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s,
> unsigned long flags;
> int locked = 1;
>
> - local_irq_save(flags);
> #if defined(SUPPORT_SYSRQ)
> - if (port->sysrq)
> + if (port->sysrq) {
> locked = 0;
> - else
> + local_irq_save(flags);
how is this helping? You should see a splat after a sysrq request.
> + } else
> #endif
> if (oops_in_progress)
> - locked = spin_trylock(&port->lock);
> + locked = spin_trylock_irqsave(&port->lock, flags);
> else
> - spin_lock(&port->lock);
> + spin_lock_irqsave(&port->lock, flags);
>
> /* first save SCSCR then disable interrupts, keep clock source */
> ctrl = serial_port_in(port, SCSCR);
Sebastian
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Sebastian Andrzej Siewior @ 2018-05-07 12:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Daniel Wagner, Linux Kernel Mailing List, linux-rt-users,
open list:SERIAL DRIVERS, Greg Kroah-Hartman, Linux-sh list,
Daniel Wagner, Shinya Kuribayashi
In-Reply-To: <CAMuHMdUjsy95zNtoZnp17dmgRcrkzfSCxTe5G=ho5nphqX0HEg@mail.gmail.com>
On 2018-05-03 09:43:33 [+0200], Geert Uytterhoeven wrote:
> > --- a/drivers/tty/serial/sh-sci.c
> > +++ b/drivers/tty/serial/sh-sci.c
> > @@ -2516,13 +2516,12 @@ static void serial_console_write(struct console *co, const char *s,
> > unsigned long flags;
> > int locked = 1;
> >
> > - local_irq_save(flags);
>
> Hence the below now runs with local interrupts enabled.
>
> For checking port->sysrq or oops_in_progress that probably isn't an issue.
> If oops_in_progress is set, you have other problems, and the race condition
> between checking the flag and calling spin_lock{,_irqsave}() existed before,
> and is hard to avoid.
while oops_in_progress is an issue of its own, the port->sysrq isn't
avoided by by local_irq_save(). On SMP systems you can still receive a
`break' signal on the UART and have a `printk()' issued on another CPU.
> For actual console printing, I think you want to keep interrupts disabled.
why? They should be disabled as part of getting the lock and not for any
other reason.
> > if (port->sysrq)
> > locked = 0;
> > else if (oops_in_progress)
> > - locked = spin_trylock(&port->lock);
> > + locked = spin_trylock_irqsave(&port->lock, flags);
> > else
> > - spin_lock(&port->lock);
> > + spin_lock_irqsave(&port->lock, flags);
>
> Add
>
> if (!locked
> local_irq_save(flags)
>
> here?
So for oops_in_progress you get here with interrupts disabled. And if
not, I don't see the point in disabling the interrupts without any kind
of locking.
> Gr{oetje,eeting}s,
>
> Geert
>
Sebastian
^ permalink raw reply
* Re: [PATCH v2] tty: implement led triggers
From: Johan Hovold @ 2018-05-07 9:27 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Johan Hovold, linux-serial, One Thousand Gnomes, Florian Fainelli,
Pavel Machek, Mathieu Poirier, Greg Kroah-Hartman, linux-kernel,
kernel, linux-arm-kernel, Robin Murphy
In-Reply-To: <20180507084127.ekpd3ze2itkzo7fd@pengutronix.de>
On Mon, May 07, 2018 at 10:41:27AM +0200, Uwe Kleine-König wrote:
> Hello Johan,
>
> thanks for your feedback.
>
> On Mon, May 07, 2018 at 10:02:52AM +0200, Johan Hovold wrote:
> > On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> > > The rx trigger fires when data is pushed to the ldisc. This is a bit later
> > > than the actual receiving of data but has the nice benefit that it
> > > doesn't need adaption for each driver and isn't in the hot path.
> > >
> > > Similarily the tx trigger fires when data taken from the ldisc.
> >
> > You meant copied from user space, or written to the ldisc, here.
>
> ack.
>
> > Note that the rx-path is shared with serdev, but the write path is not.
> > So with this series, serdev devices would only trigger the rx-led.
>
> Where would be the right place to put the tx trigger to catch serdev,
> too?
I haven't given this much thought, but do we really want this for serdev
at all? I'm thinking whatever driver or subsystem is using serdev should
have their own triggers (e.g. bluetooth or net).
So it might be better to move the rx-blinking to the default tty-port
client receive_buf callback instead (i.e.
tty_port_default_receive_buf()).
And then the resource allocations would need to go after the serdev
registration in tty_port_register_device_attr_serdev().
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Changes since v1, sent with Message-Id:
> > > 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
> > >
> > > - implement tx trigger;
> > > - introduce Kconfig symbol for conditional compilation;
> > > - set trigger to NULL if allocating the name failed to not free random
> > > pointers in case the port struct wasn't zeroed;
> > > - use if/else instead of goto
> >
> > > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > > struct tty_buffer *head = buf->head;
> > > struct tty_buffer *next;
> > > int count;
> > > + unsigned long delay = 50 /* ms */;
> >
> > Comment after the semicolon?
>
> Given that this comment is about the 50 and not the delay member, I
> prefer it before the ;.
Hmm. I personally find it hard to read and can only find about 30
instances of this comment style (for assignments) in the kernel. And
arguably the comment applies equally well to the delay variable in this
case too.
> > Besides the ugly ifdefs here, you're leaking the above LED resources in
> > the error paths below (e.g. on probe deferrals).
> ack, the ifdevs are ugly. I'm working on an idea to get rid of them.
Sounds good.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH v2] tty: implement led triggers
From: Uwe Kleine-König @ 2018-05-07 8:41 UTC (permalink / raw)
To: Johan Hovold
Cc: linux-serial, One Thousand Gnomes, Florian Fainelli, Pavel Machek,
Mathieu Poirier, Greg Kroah-Hartman, linux-kernel, kernel,
linux-arm-kernel, Robin Murphy
In-Reply-To: <20180507080252.GO2285@localhost>
Hello Johan,
thanks for your feedback.
On Mon, May 07, 2018 at 10:02:52AM +0200, Johan Hovold wrote:
> On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> > The rx trigger fires when data is pushed to the ldisc. This is a bit later
> > than the actual receiving of data but has the nice benefit that it
> > doesn't need adaption for each driver and isn't in the hot path.
> >
> > Similarily the tx trigger fires when data taken from the ldisc.
>
> You meant copied from user space, or written to the ldisc, here.
ack.
> Note that the rx-path is shared with serdev, but the write path is not.
> So with this series, serdev devices would only trigger the rx-led.
Where would be the right place to put the tx trigger to catch serdev,
too?
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Changes since v1, sent with Message-Id:
> > 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
> >
> > - implement tx trigger;
> > - introduce Kconfig symbol for conditional compilation;
> > - set trigger to NULL if allocating the name failed to not free random
> > pointers in case the port struct wasn't zeroed;
> > - use if/else instead of goto
>
> > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > struct tty_buffer *head = buf->head;
> > struct tty_buffer *next;
> > int count;
> > + unsigned long delay = 50 /* ms */;
>
> Comment after the semicolon?
Given that this comment is about the 50 and not the delay member, I
prefer it before the ;.
> >
> > /* Ldisc or user is trying to gain exclusive access */
> > if (atomic_read(&buf->priority))
>
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 25d736880013..f042879a597c 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -16,6 +16,7 @@
> > #include <linux/wait.h>
> > #include <linux/bitops.h>
> > #include <linux/delay.h>
> > +#include <linux/leds.h>
> > #include <linux/module.h>
> > #include <linux/serdev.h>
> >
> > @@ -157,6 +158,30 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> >
> > tty_port_link_device(port, driver, index);
> >
> > +#ifdef CONFIG_TTY_LEDS_TRIGGER
> > + port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> > + driver->name, index);
> > + if (port->led_trigger_rx_name) {
> > + led_trigger_register_simple(port->led_trigger_rx_name,
> > + &port->led_trigger_rx);
> > + } else {
> > + port->led_trigger_rx = NULL;
> > + pr_err("Failed to allocate trigger name for %s%d\n",
> > + driver->name, index);
> > + }
> > +
> > + port->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
> > + driver->name, index);
> > + if (port->led_trigger_tx_name) {
> > + led_trigger_register_simple(port->led_trigger_tx_name,
> > + &port->led_trigger_tx);
> > + } else {
> > + port->led_trigger_tx = NULL;
> > + pr_err("Failed to allocate trigger name for %s%d\n",
> > + driver->name, index);
> > + }
> > +#endif
>
> Besides the ugly ifdefs here, you're leaking the above LED resources in
> the error paths below (e.g. on probe deferrals).
ack, the ifdevs are ugly. I'm working on an idea to get rid of them.
Will think about how to plug the leak.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v2] tty: implement led triggers
From: Johan Hovold @ 2018-05-07 8:02 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-serial, One Thousand Gnomes, Florian Fainelli, Pavel Machek,
Mathieu Poirier, Greg Kroah-Hartman, linux-kernel, kernel,
linux-arm-kernel, Robin Murphy
In-Reply-To: <20180503201952.16592-1-u.kleine-koenig@pengutronix.de>
On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> The rx trigger fires when data is pushed to the ldisc. This is a bit later
> than the actual receiving of data but has the nice benefit that it
> doesn't need adaption for each driver and isn't in the hot path.
>
> Similarily the tx trigger fires when data taken from the ldisc.
You meant copied from user space, or written to the ldisc, here.
Note that the rx-path is shared with serdev, but the write path is not.
So with this series, serdev devices would only trigger the rx-led.
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since v1, sent with Message-Id:
> 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
>
> - implement tx trigger;
> - introduce Kconfig symbol for conditional compilation;
> - set trigger to NULL if allocating the name failed to not free random
> pointers in case the port struct wasn't zeroed;
> - use if/else instead of goto
> @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> struct tty_buffer *head = buf->head;
> struct tty_buffer *next;
> int count;
> + unsigned long delay = 50 /* ms */;
Comment after the semicolon?
>
> /* Ldisc or user is trying to gain exclusive access */
> if (atomic_read(&buf->priority))
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 25d736880013..f042879a597c 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -16,6 +16,7 @@
> #include <linux/wait.h>
> #include <linux/bitops.h>
> #include <linux/delay.h>
> +#include <linux/leds.h>
> #include <linux/module.h>
> #include <linux/serdev.h>
>
> @@ -157,6 +158,30 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
>
> tty_port_link_device(port, driver, index);
>
> +#ifdef CONFIG_TTY_LEDS_TRIGGER
> + port->led_trigger_rx_name = kasprintf(GFP_KERNEL, "%s%d-rx",
> + driver->name, index);
> + if (port->led_trigger_rx_name) {
> + led_trigger_register_simple(port->led_trigger_rx_name,
> + &port->led_trigger_rx);
> + } else {
> + port->led_trigger_rx = NULL;
> + pr_err("Failed to allocate trigger name for %s%d\n",
> + driver->name, index);
> + }
> +
> + port->led_trigger_tx_name = kasprintf(GFP_KERNEL, "%s%d-tx",
> + driver->name, index);
> + if (port->led_trigger_tx_name) {
> + led_trigger_register_simple(port->led_trigger_tx_name,
> + &port->led_trigger_tx);
> + } else {
> + port->led_trigger_tx = NULL;
> + pr_err("Failed to allocate trigger name for %s%d\n",
> + driver->name, index);
> + }
> +#endif
Besides the ugly ifdefs here, you're leaking the above LED resources in
the error paths below (e.g. on probe deferrals).
> dev = serdev_tty_port_register(port, device, driver, index);
> if (PTR_ERR(dev) != -ENODEV) {
> /* Skip creating cdev if we registered a serdev device */
> @@ -249,6 +249,13 @@ struct tty_port {
> set to size of fifo */
> struct kref kref; /* Ref counter */
> void *client_data;
> +
> +#ifdef CONFIG_TTY_LEDS_TRIGGER
> + struct led_trigger *led_trigger_rx;
> + char *led_trigger_rx_name;
const?
> + struct led_trigger *led_trigger_tx;
> + char *led_trigger_tx_name;
ditto.
> +#endif
Johan
^ permalink raw reply
* Re: [PATCH v3 12/20] media: Remove depends on HAS_DMA in case of platform dependency
From: Geert Uytterhoeven @ 2018-05-07 7:21 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Ulf Hansson, Wolfram Sang, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-fpga-u79uwXL29TY76Z2rM5mHXA,
open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM,
ALSA Development Mailing List, Bjorn Andersson, Eric Anholt,
netdev, MTD Maling List, Linux I2C,
linux1394-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
Christoph Hellwig, Stefan Wahren, Boris Brezillon, Herbert Xu,
Richard Weinberger, Jassi Brar, Marek Vasut,
open list:SERIAL DRIVERS, Matias Bjorling
In-Reply-To: <20180505094743.4d201ba0-ch4gOOMV7nf/PtFMR13I2A@public.gmane.org>
Hi Mauro,
On Sat, May 5, 2018 at 2:47 PM, Mauro Carvalho Chehab
<mchehab+samsung-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Em Tue, 17 Apr 2018 19:49:12 +0200
> Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> escreveu:
>
>> Remove dependencies on HAS_DMA where a Kconfig symbol depends on another
>> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
>
> Applying a patch like that is hard, as there are lots of churn at
> the code. That's against latest media upstream:
[...]
I can rebase. What's your preferred target? media-next?
>> In most cases this other symbol is an architecture or platform specific
>> symbol, or PCI.
>>
>> Generic symbols and drivers without platform dependencies keep their
>> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
>> cannot work anyway.
>
> Actually, depends on HAS_DMA was introduced on media because builds
> were failing otherwise. We started adding it before the addition
> of COMPILE_TEST.
Indeed. Before the 5 commits below, a missing dependency on HAS_DMA
would cause the build to fail for NO_DMA platforms.
f29ab49b5388b2f8 dma-mapping: Convert NO_DMA get_dma_ops() into a real dummy
ab642e952f80c66c dma-coherent: Add NO_DMA dummies for managed DMA API
c1ce6c2beea38171 mm: Add NO_DMA dummies for DMA pool API
bff739b6559e2fce scsi: Add NO_DMA dummies for SCSI DMA mapping API
1f674e16f9ce6eb2 usb: gadget: Add NO_DMA dummies for DMA mapping API
> Can we just remove all HAS_DMA Kconfig dependencies as a hole from the
> entire media subsystem, with something like:
>
> $ for i in $(find drivers/media -name Kconfig) $(find drivers/staging/media -name Kconfig); do sed '/depends on HAS_DMA/d;s/ && HAS_DMA//g' -i $i; done
>
> Or would it cause build issues?
It should not cause build issues, but it would enable lots of drivers that
need DMA platform support on platforms where they wouldn't work, making the
life of distro configurators harder.
That's why my series removes HAS_DMA only for symbols that have a platform
dependency.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: Use spin_{try}lock_irqsave instead of open coding version
From: Geert Uytterhoeven @ 2018-05-07 7:14 UTC (permalink / raw)
To: Daniel Wagner
Cc: Linux Kernel Mailing List, linux-rt-users,
open list:SERIAL DRIVERS, Linux-sh list, Greg KH, Daniel Wagner
In-Reply-To: <20180504163041.28726-1-wagi@monom.org>
Hi Daniel,
On Fri, May 4, 2018 at 6:30 PM, Daniel Wagner <wagi@monom.org> wrote:
> From: Daniel Wagner <daniel.wagner@siemens.com>
>
> Commit 40f70c03e33a ("serial: sh-sci: add locking to console write
> function to avoid SMP lockup") copied the strategy to avoid locking
> problems in conjuncture with the console from the UART8250
> driver. Instead using directly spin_{try}lock_irqsave(),
> local_irq_save() followed by spin_{try}lock() was used. While this is
> correct on mainline, for -rt it is a problem. spin_{try}lock() will
> check if it is running in a valid context. Since the local_irq_save()
> has already been executed, the context has changed and
> spin_{try}lock() will complain. The reason why spin_{try}lock()
> complains is that on -rt the spin locks are turned into mutexes and
> therefore can sleep. Sleeping with interrupts disabled is not valid.
[...]
> ---
>
> changes since v1:
> - Ported to current mainline (initial version was against v4.4.y)
> - Left local_irq_save() in place when spinlocks are not used as suggested
> by Geert.
Thanks for the update!
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2890,16 +2890,16 @@ static void serial_console_write(struct console *co, const char *s,
> unsigned long flags;
> int locked = 1;
>
> - local_irq_save(flags);
> #if defined(SUPPORT_SYSRQ)
> - if (port->sysrq)
> + if (port->sysrq) {
> locked = 0;
> - else
> + local_irq_save(flags);
> + } else
> #endif
> if (oops_in_progress)
> - locked = spin_trylock(&port->lock);
> + locked = spin_trylock_irqsave(&port->lock, flags);
If the spinlock could not be taken, interrupts are re-enabled:
include/linux/spinlock.h:
#define raw_spin_trylock_irqsave(lock, flags) \
({ \
local_irq_save(flags); \
raw_spin_trylock(lock) ? \
1 : ({ local_irq_restore(flags); 0; }); \
})
hence I think you need to check for this and disable interrupts again:
if (!locked)
local_irq_save(flags);
> else
> - spin_lock(&port->lock);
> + spin_lock_irqsave(&port->lock, flags);
>
> /* first save SCSCR then disable interrupts, keep clock source */
> ctrl = serial_port_in(port, SCSCR);
> @@ -2919,8 +2919,9 @@ static void serial_console_write(struct console *co, const char *s,
> serial_port_out(port, SCSCR, ctrl);
>
> if (locked)
> - spin_unlock(&port->lock);
> - local_irq_restore(flags);
> + spin_unlock_irqrestore(&port->lock, flags);
> + else
> + local_irq_restore(flags);
> }
>
> static int serial_console_setup(struct console *co, char *options)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [RFC PATCH 0/3] serial: uartps: Add run time support for more IPs than hardcoded 2
From: Maarten Brock @ 2018-05-05 13:10 UTC (permalink / raw)
To: Michal Simek
Cc: gnomes, monstr, linux-serial-owner, devicetree,
Greg Kroah-Hartman, linux-kernel, Alexander Graf, Rob Herring,
linux-serial, Jiri Slaby, Frank Rowand, linux-arm-kernel
In-Reply-To: <cover.1524751696.git.michal.simek@xilinx.com>
On 2018-04-26 16:08, Michal Simek wrote:
> Hi,
>
> this series is trying to address discussion I had with Alan in past
> https://patchwork.kernel.org/patch/9738445/.
>
> It is moving uart_register_driver() to probe function like it is done
> in
> pl011 driver.
> And also introducing new function for alias compatibility checking to
> resolve cases where some IPs have alias and some of them not.
> This case is detected in pl011_probe_dt_alias() but not properly
> solved.
>
> Also keep status of free ids/minor numbers in bitmap to know what's the
> next unallocated number.
>
> The same solution can be used in pl011, uart16550 and uartlite to
> really
> get unified kernel.
>
> Tested on these use cases:
> Notes:
> ff000000 is the first PS uart listed in dtsi
> ff010000 is the second PS uart listed in dtsi
>
> Standard zcu102 setting
> serial0 = &uart0;
> serial1 = &uart1;
> [ 0.196628] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.642542] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> Swapped zcu102 setting
> serial0 = &uart1;
> serial1 = &uart0;
> [ 0.196472] ff000000.serial: ttyPS1 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.196824] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> uart0 on alias higher then MAX uart ports
> serial0 = &uart1;
> serial200 = &uart0;
> [ 0.176793] ff000000.serial: ttyPS200 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.177288] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> Both uarts on higher aliases
> serial1 = &uart1;
> serial2 = &uart0;
> [ 0.196470] ff000000.serial: ttyPS2 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.196823] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> uart0 not listed but it is probed first that's why should be ttyPS0 but
> there is uart1 via alias
> serial0 = &uart1;
> [ 0.176656] xuartps ff000000.serial: No serial alias passed. Using
> the first free id
> [ 0.176671] xuartps ff000000.serial: Validate id 0
> [ 0.176680] xuartps ff000000.serial: The empty id is 0
> [ 0.176692] xuartps ff000000.serial: ID 0 already taken - skipped
> [ 0.176701] xuartps ff000000.serial: Validate id 1
> [ 0.176710] xuartps ff000000.serial: The empty id is 1
> [ 0.176719] xuartps ff000000.serial: Selected ID 1 allocation passed
> [ 0.176760] ff000000.serial: ttyPS1 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.177104] ff010000.serial: ttyPS0 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> uart0 not listed but it is probed first that's why should be ttyPS0
> serial1 = &uart1;
> [ 0.176661] xuartps ff000000.serial: No serial alias passed. Using
> the first free id
> [ 0.176676] xuartps ff000000.serial: Validate id 0
> [ 0.176686] xuartps ff000000.serial: The empty id is 0
> [ 0.176696] xuartps ff000000.serial: Selected ID 0 allocation passed
> [ 0.176737] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.177069] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> uarts not listed in aliases list
> [ 0.176673] xuartps ff000000.serial: No serial alias passed. Using
> the first free id
> [ 0.176687] xuartps ff000000.serial: Validate id 0
> [ 0.176697] xuartps ff000000.serial: The empty id is 0
> [ 0.176707] xuartps ff000000.serial: Selected ID 0 allocation passed
> [ 0.176746] ff000000.serial: ttyPS0 at MMIO 0xff000000 (irq = 33,
> base_baud = 6250000) is a xuartps
> [ 0.177057] xuartps ff010000.serial: No serial alias passed. Using
> the first free id
> [ 0.177070] xuartps ff010000.serial: Validate id 0
> [ 0.177079] xuartps ff010000.serial: The empty id is 0
> [ 0.177089] xuartps ff010000.serial: Selected ID 0 allocation failed
> [ 0.177098] xuartps ff010000.serial: Validate id 1
> [ 0.177107] xuartps ff010000.serial: The empty id is 1
> [ 0.177116] xuartps ff010000.serial: Selected ID 1 allocation passed
> [ 0.177149] ff010000.serial: ttyPS1 at MMIO 0xff010000 (irq = 34,
> base_baud = 6250000) is a xuartps
>
> Thanks,
> Michal
Hello Michal,
How will this interact with ns16550 based UARTs?
Can we have both /dev/ttyS0 and /dev/ttyPS0?
Currently we can't unless we create *no* serialN alias for the ns16550.
And there is no other means to lock the /dev/ttySx name to a device
either.
Or will the xuartps driver eventually use /dev/ttySx as well?
Maarten
^ permalink raw reply
* Re: [PATCH v3 12/20] media: Remove depends on HAS_DMA in case of platform dependency
From: Mauro Carvalho Chehab @ 2018-05-05 12:47 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Ulf Hansson, Wolfram Sang, linux-iio, linux-fpga,
linux-remoteproc, alsa-devel, Bjorn Andersson, Eric Anholt,
netdev, linux-mtd, linux-i2c, linux1394-devel, Christoph Hellwig,
Marek Szyprowski, Stefan Wahren, Boris Brezillon, Herbert Xu,
Richard Weinberger, Joerg Roedel, Jassi Brar, Marek Vasut,
linux-serial, Matias Bjorling, David Woodhouse, linux-media, Ohad
In-Reply-To: <1523987360-18760-13-git-send-email-geert@linux-m68k.org>
Hi Geert,
Em Tue, 17 Apr 2018 19:49:12 +0200
Geert Uytterhoeven <geert@linux-m68k.org> escreveu:
> Remove dependencies on HAS_DMA where a Kconfig symbol depends on another
> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
Applying a patch like that is hard, as there are lots of churn at
the code. That's against latest media upstream:
checking file drivers/media/pci/intel/ipu3/Kconfig
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED
checking file drivers/media/pci/solo6x10/Kconfig
checking file drivers/media/pci/sta2x11/Kconfig
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED
checking file drivers/media/pci/tw5864/Kconfig
checking file drivers/media/pci/tw686x/Kconfig
checking file drivers/media/platform/Kconfig
Hunk #2 FAILED at 63.
Hunk #3 succeeded at 81 (offset 1 line).
Hunk #4 succeeded at 91 (offset 1 line).
Hunk #5 succeeded at 101 (offset 1 line).
Hunk #6 succeeded at 111 (offset 1 line).
Hunk #7 succeeded at 124 (offset 1 line).
Hunk #8 succeeded at 142 (offset 1 line).
Hunk #9 succeeded at 169 (offset 1 line).
Hunk #10 succeeded at 186 (offset 1 line).
Hunk #11 succeeded at 197 (offset 1 line).
Hunk #12 succeeded at 213 (offset 1 line).
Hunk #13 succeeded at 227 (offset 1 line).
Hunk #14 succeeded at 254 (offset 1 line).
Hunk #15 succeeded at 265 (offset 1 line).
Hunk #16 succeeded at 275 (offset 1 line).
Hunk #17 succeeded at 284 (offset 1 line).
Hunk #18 succeeded at 295 (offset 1 line).
Hunk #19 succeeded at 303 (offset 1 line).
Hunk #20 succeeded at 312 (offset 1 line).
Hunk #21 succeeded at 338 (offset 1 line).
Hunk #22 succeeded at 383 (offset 1 line).
Hunk #23 succeeded at 397 (offset 1 line).
Hunk #24 succeeded at 422 (offset 1 line).
Hunk #25 succeeded at 435 (offset 1 line).
Hunk #26 succeeded at 452 (offset 1 line).
Hunk #27 succeeded at 470 (offset 1 line).
Hunk #28 succeeded at 612 (offset 1 line).
1 out of 28 hunks FAILED
> In most cases this other symbol is an architecture or platform specific
> symbol, or PCI.
>
> Generic symbols and drivers without platform dependencies keep their
> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
> cannot work anyway.
Actually, depends on HAS_DMA was introduced on media because builds
were failing otherwise. We started adding it before the addition
of COMPILE_TEST.
Can we just remove all HAS_DMA Kconfig dependencies as a hole from the
entire media subsystem, with something like:
$ for i in $(find drivers/media -name Kconfig) $(find drivers/staging/media -name Kconfig); do sed '/depends on HAS_DMA/d;s/ && HAS_DMA//g' -i $i; done
Or would it cause build issues?
Regards,
Mauro
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH v3 3/3] sh: add the sh_ prefix to early platform symbols
From: Arnd Bergmann @ 2018-05-05 0:04 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
Stephen Boyd, Greg Kroah-Hartman, Mark Rutland, Yoshinori Sato,
Rich Felker, Andy Shevchenko, Marc Zyngier, Rafael J . Wysocki,
Peter Rosin, Jiri Slaby, Thomas Gleixner, Daniel Lezcano,
Geert Uytterhoeven, Magnus Damm, Linux ARM
In-Reply-To: <20180504132731.14574-4-brgl@bgdev.pl>
On Fri, May 4, 2018 at 9:27 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Old early platform device support is now sh-specific. Before moving on
> to implementing new early platform framework based on real platform
> devices, prefix all early platform symbols with 'sh_'.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Looks good. I was thinking of something shorter as the new namespace,
but actually using a longer name like you do seems appropriate since
we don't want anyone to actually use it. ;-)
Arnd
^ permalink raw reply
* Re: [PATCH v3 1/3] clocksource: timer-ti-dm: remove the early platform driver registration
From: Arnd Bergmann @ 2018-05-05 0:02 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Sekhar Nori, Kevin Hilman, David Lechner, Michael Turquette,
Stephen Boyd, Greg Kroah-Hartman, Mark Rutland, Yoshinori Sato,
Rich Felker, Andy Shevchenko, Marc Zyngier, Rafael J . Wysocki,
Peter Rosin, Jiri Slaby, Thomas Gleixner, Daniel Lezcano,
Geert Uytterhoeven, Magnus Damm, Linux ARM
In-Reply-To: <20180504132731.14574-2-brgl@bgdev.pl>
On Fri, May 4, 2018 at 9:27 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This driver is no longer used as an early platform driver. Remove the
> registration macro.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply
* Re: [PATCH v2] tty/serial: atmel: use port->name as name in request_irq()
From: Rob Herring @ 2018-05-04 20:23 UTC (permalink / raw)
To: Richard Genoud
Cc: Alexandre Belloni, Peter Hurley, Greg Kroah-Hartman,
Sebastian Andrzej Siewior, open list:SERIAL DRIVERS, Jiri Slaby,
Thomas Gleixner,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <d62a945c-f81e-d7e5-8431-a7948a0b833c@sorico.fr>
On Fri, May 4, 2018 at 5:28 AM, Richard Genoud <richard.genoud@gmail.com> wrote:
> On 04/05/2018 10:14, Sebastian Andrzej Siewior wrote:
>> I was puzzled while looking at /proc/interrupts and random things showed
>> up between reboots. This occurred more often but I realised it later. The
>> "correct" output should be:
>> |38: 11861 atmel-aic5 2 Level ttyS0
>>
>> but I saw sometimes
>> |38: 6426 atmel-aic5 2 Level tty1
>>
>> and accounted it wrongly as correct. This is use after free and the
>> former example randomly got the "old" pointer which pointed to the same
>> content. With SLAB_FREELIST_RANDOM and HARDENED I even got
>> |38: 7067 atmel-aic5 2 Level E=Started User Manager for UID 0
>>
>> or other nonsense.
>> As it turns out the tty, pointer that is accessed in atmel_startup(), is
>> freed() before atmel_shutdown(). It seems to happen quite often that the
>> tty for ttyS0 is allocated and freed while ->shutdown is not invoked. I
>> don't do anything special - just a systemd boot :)
>>
>> Use port->name as the IRQ name for request_irq(). This exists as long as
>> the driver is loaded so no use-after-free here.
>> For backports before v4.12 I suggest to use `"atmel_serial"' instead
>> `port->name' (that member was introduced in f7048b15900f ("tty: serial_core:
>> Add name field to uart_port struct").
>>
>> Cc: stable@vger.kernel.org
> I think it's safer to use:
> Cc: stable@vger.kernel.org # 4.14
> Because the stable team may miss your comment, and even if they don't, I
> think it's not their role to adapt the patch to 4.9.x (and test it !)
> IMHO, the best way is to add # 4.14 and when it's applied on 4.14.x,
> send a tested backport for 4.9.x
>
> Besides that, you can add my:
> Acked-by: Richard Genoud <richard.genoud@gmail.com>
>
> Rob, do you agree with this fix ?
Yes, one less dependency on tty struct in serial drivers is a good
thing. However, I find port->name to be a somewhat pointless addition.
Most platform drivers (which most serial drivers are) use the device
name for registering their IRQ. And now serial drivers may not even
have a tty with serdev. Using the device name would also solve your
backporting issue. But either way:
Acked-by: Rob Herring <robh@kernel.org>
Rob
^ permalink raw reply
* Re: [PATCHv2] serial: 8250: omap: Fix idling of clocks for unused uarts
From: Tony Lindgren @ 2018-05-04 17:58 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Peter Hurley, Greg Kroah-Hartman, Peter Ujfalusi, Vignesh R,
linux-serial, linux-omap, linux-kernel, Keerthy,
Matthijs van Duin, Sekhar Nori, Tero Kristo
In-Reply-To: <20180504174826.b7hls2c6v7gukdgl@linutronix.de>
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [180504 17:50]:
> On 2018-05-04 10:39:31 [-0700], Tony Lindgren wrote:
> > Uhh sorry I managed to commit also a patch I was testing while
> > updating patch comments.. Will send out v3 shortly, this can be
> > ignored.
>
> I assumed you were helping out to sneak a patch in.
Heheh, at least it proves I did have it applied before I replied
with my Boot-tested-for-few-times-by :)
Tony
^ permalink raw reply
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