* Re: [[PATCH] 0/9] *** DMA support for UART in ASPEED's AST2500 ***
From: sudheer.v @ 2018-10-26 7:23 UTC (permalink / raw)
To: Rob Herring
Cc: Vinod Koul, Mark Rutland, Greg Kroah-Hartman, Joel Stanley,
Andrew Jeffery, Russell King, Dan Williams, Jiri Slaby,
Thomas Gleixner, Marc Zyngier, Benjamin Herrenschmidt,
Christian Borntraeger, Michael Moese, Hendrik Brueckner,
Kate Stewart, Philippe Ombredanne, dmaengine, devicetree,
linux-kernel, linux-serial, linux-arm
In-Reply-To: <20181025144841.GA21004@bogus>
On Thu, Oct 25, 2018 at 09:48:41AM -0500, Rob Herring wrote:
> On Wed, Oct 17, 2018 at 09:40:57AM +0530, sudheer.v wrote:
> > DMA controller driver and UART dma client driver for aspeed's AST2500
> >
> > sudheer.v (9):
>
> We need a full name for author and Signed-off-by.
>
> > DT-changes-for-DMA-UART-of-AST2500
> > Defconfig-changes-for-DMA-UART-of-AST2500
> > configuration-for-DMA-of-AST2500
> > Documentation-DTbindings-DMA-controller-of-AST2500
> > DMA-driver-for-AST2500
> > configuration-for-DMA-UART-of-AST2500
> > Documentation-DTbindings-DMA-UART-of-AST2500
> > DMA-UART-Driver-for-AST2500
> > updating MAINTAINERS for DMA and DMA-UART drivers of AST2500
>
> Your commit messages need some work. Use 'git log --oneline <dir>' on
> each directory for inspiration as to what the subject should be.
>
> DT bindings specifically are "dt-bindings: <binding dir>: ..."
>
> Rob
Hi Rob,
As per the comments given by vinod and Ben,
we are planning to resubmit the patches, after merging these
two drivers (dma and uart) into single driver.
While resubmitting, will ensure patches are formatted with
proper messages, following the convention.
Regards
Sudheer
^ permalink raw reply
* Re: [[PATCH] 8/9] DMA-UART-Driver-for-AST2500
From: sudheer.v @ 2018-10-26 7:07 UTC (permalink / raw)
To: Vinod
Cc: Benjamin Herrenschmidt, Rob Herring, Mark Rutland,
Greg Kroah-Hartman, Joel Stanley, Andrew Jeffery, Russell King,
Dan Williams, Jiri Slaby, Thomas Gleixner, Marc Zyngier,
Christian Borntraeger, Michael Moese, Hendrik Brueckner,
Kate Stewart, Philippe Ombredanne, dmaengine, devicetree,
linux-kernel, linux-serial
In-Reply-To: <20181020162624.GC2894@vkoul-mobl>
On Sat, Oct 20, 2018 at 09:56:24PM +0530, Vinod wrote:
> On 19-10-18, 12:41, sudheer.v wrote:
> > On Fri, Oct 19, 2018 at 10:32:24AM +1100, Benjamin Herrenschmidt wrote:
> > > On Thu, 2018-10-18 at 15:25 +0530, Vinod wrote:
> > > >
> > > > > It's not a dmaengine driver. It's a serial UART driver that happens to
> > > > > use a dedicated DMA engine.
> > > >
> > > > Then I see no reason for it to use dmaengine APIs. The framework allows
> > > > people to share a controller for many clients, but if you have dedicated
> > > > one then you may use it directly
> > >
> > > Well... the engine is shared by a few UARTs, they have dedicated rings
> > > but there's a common set of regs for interrupt handling etc.
> > >
> > > That said, I still think it could be contained within a UART driver,
> > > there's little benefit in adding the framework overhead, esp since
> > > these are really weak cores, any overhead will be felt.
> > >
> > > Ben.
> > >
> > > > > It's unclear whether it should be split into two drivers, or just have
> > > > > the serial driver directly use the dma engine since that engine is
> > > > > dedicated in HW to only work on those UARTs and nothing else...
> > > > >
> > > > > Cheers,
> > > > > Ben.
> >
> > Initially we wanted to have a single driver,
> > however we had an informal discussion with one of the maintainer
> > and based on the feedback, followed the Linux DMA and UART architecture.
> >
> > If this seperate DMA-engine driver adds more overhead than benifit,
> > we will merge them into a single UART driver and resubmitt the patches.
> > Vinod,
> > can this dma-controller driver sit under dma subsystem?.
> > or better to move it under UART framework.
>
>
> My advise would be to see what you can do with the DMA IP block. If this
> can/would be used in different places then it would make sense to do a
> dmaengine driver and solve the problem for everyone.
>
> If this is always going to be hidden behind serial then maybe it makes
> sense to be inside serial driver and not use dmaengine APIs
>
> If you decide to prefer the former case, please move it to dmaengine and
> resubmit :)
>
> HTH
> --
> ~Vinod
Hi All,
As the DMA engine is dedicated only to UART,we have decided
to rewrite the driver so that no code will come under
drivers/dma.
I will resubmitt the patches after merging dma controller
code and uart driver code.
Regards
-sudheer
^ permalink raw reply
* Re: [[PATCH] 0/9] *** DMA support for UART in ASPEED's AST2500 ***
From: Rob Herring @ 2018-10-25 14:48 UTC (permalink / raw)
To: sudheer.v
Cc: Vinod Koul, Mark Rutland, Greg Kroah-Hartman, Joel Stanley,
Andrew Jeffery, Russell King, Dan Williams, Jiri Slaby,
Thomas Gleixner, Marc Zyngier, Benjamin Herrenschmidt,
Christian Borntraeger, Michael Moese, Hendrik Brueckner,
Kate Stewart, Philippe Ombredanne, dmaengine, devicetree,
linux-kernel, linux-serial, linux-arm
In-Reply-To: <1539749466-3912-1-git-send-email-open.sudheer@gmail.com>
On Wed, Oct 17, 2018 at 09:40:57AM +0530, sudheer.v wrote:
> DMA controller driver and UART dma client driver for aspeed's AST2500
>
> sudheer.v (9):
We need a full name for author and Signed-off-by.
> DT-changes-for-DMA-UART-of-AST2500
> Defconfig-changes-for-DMA-UART-of-AST2500
> configuration-for-DMA-of-AST2500
> Documentation-DTbindings-DMA-controller-of-AST2500
> DMA-driver-for-AST2500
> configuration-for-DMA-UART-of-AST2500
> Documentation-DTbindings-DMA-UART-of-AST2500
> DMA-UART-Driver-for-AST2500
> updating MAINTAINERS for DMA and DMA-UART drivers of AST2500
Your commit messages need some work. Use 'git log --oneline <dir>' on
each directory for inspiration as to what the subject should be.
DT bindings specifically are "dt-bindings: <binding dir>: ..."
Rob
^ permalink raw reply
* Re: Crash in msm serial on dragonboard with ftrace bootargs
From: saiprakash.ranjan @ 2018-10-25 14:36 UTC (permalink / raw)
To: Steven Rostedt, srinivas.kandagatla, Stephen Boyd
Cc: Joel Fernandes, Bjorn Andersson, Andy Gross, David Brown,
Jiri Slaby, Kees Cook, Geliang Tang, Greg Kroah-Hartman,
Pramod Gurav, linux-arm-msm, linux-soc, linux-serial,
linux-kernel, Rajendra Nayak, Vivek Gautam, Sibi Sankar
In-Reply-To: <20181019111205.5c8e98e8@gandalf.local.home>
On 10/19/2018 8:42 PM, Steven Rostedt wrote:
> On Fri, 19 Oct 2018 20:18:19 +0530
> Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org> wrote:
>
>
>>> You said that if you add 'ftrace_filter=msm_read' to the command
>>> line,
>>> it still crashes?
>>>
>>> So only tracing that function we have an issue, right?
>>>
>>
>> Tracing msm_read does cause the crash, but that is not the only one
>> causing the crash because even after "ftrace_notrace=msm_read" the
>> board
>> crashes which is why I suspect msm earlycon and not ftrace.
>
> I'm saying there's a combination of the two. Because when ftrace is not
> involved, early con doesn't crash.
>
> I would focus on why earlycon crashes when only msm_read is traced.
> That should help narrow down the cause.
>
> What we know is:
>
> earlycon -> boots
>
> ftrace=function -> boots
>
> earlycon ftrace=function ftrace_filter=msm_read -> crashes
>
> If we remove the "ftrace=function ftrace_filter=msm_read" it boots
> fine, and I'm assuming that if we just remove earlycon (keeping the
> ftrace_filter), it also boots too.
>
> Thus, ftrace tracing msm_read is causing earlycon to do something that
> triggers the crash.
>
Hi,
Sorry for late response on this, was running out of debug options
;-)
Finally after bisecting (which I should have done earlier) this issue
till 4.4 kernel where I don't see the crash, I could see that
dma node in "apq8016-sbc.dtsi" is somehow causing the crash.
dma@7884000 {
status = "okay"; <--- This one
};
serial@78af000 {
label = "LS-UART0";
status = "okay";
pinctrl-names = "default", "sleep";
pinctrl-0 = <&blsp1_uart1_default>;
pinctrl-1 = <&blsp1_uart1_sleep>;
};
serial@78b0000 {
label = "LS-UART1";
status = "okay";
pinctrl-names = "default", "sleep";
pinctrl-0 = <&blsp1_uart2_default>;
pinctrl-1 = <&blsp1_uart2_sleep>;
};
"If I disable dma node and LS-UART0, then I don't see any crash and
ftrace also works fine"
And one more observation is that even without ftrace cmdline, if I use
earlycon and disable dma, I face the same crash.
So basically this seems to be some kind of earlycon and dma issue and
not ftrace(I can be wrong).
So adding Srinivas for more info on this dma node.
Also just for a note: apq8096-db820c.dtsi shows UART0 is disabled
because
bootloader does not allow access to it. Could this also be the case for
db410c?
Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* [PATCH AUTOSEL 3.18 34/98] serial: samsung: Add the support for Exynos5433 SoC
From: Sasha Levin @ 2018-10-25 14:17 UTC (permalink / raw)
To: stable, linux-kernel
Cc: Chanwoo Choi, Greg Kroah-Hartman, Jiri Slaby, linux-serial,
Sasha Levin
In-Reply-To: <20181025141853.214051-1-sashal@kernel.org>
From: Chanwoo Choi <cw00.choi@samsung.com>
[ Upstream commit 31ec77aca72ee5920ed3ec3d047734dc0bc43342 ]
This patch adds new s3c24xx_serial_drv_data structure for Exynos5433 SoC
because Exynos5433 has different fifo size from existing Exynos4 SoC.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
Acked-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Geunsik Lim <geunsik.lim@samsung.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/tty/serial/samsung.c | 56 +++++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
index ba59a76edc8b..957992ceba0a 100644
--- a/drivers/tty/serial/samsung.c
+++ b/drivers/tty/serial/samsung.c
@@ -1785,32 +1785,43 @@ static struct s3c24xx_serial_drv_data s5pv210_serial_drv_data = {
#endif
#if defined(CONFIG_ARCH_EXYNOS)
+#define EXYNOS_COMMON_SERIAL_DRV_DATA \
+ .info = &(struct s3c24xx_uart_info) { \
+ .name = "Samsung Exynos UART", \
+ .type = PORT_S3C6400, \
+ .has_divslot = 1, \
+ .rx_fifomask = S5PV210_UFSTAT_RXMASK, \
+ .rx_fifoshift = S5PV210_UFSTAT_RXSHIFT, \
+ .rx_fifofull = S5PV210_UFSTAT_RXFULL, \
+ .tx_fifofull = S5PV210_UFSTAT_TXFULL, \
+ .tx_fifomask = S5PV210_UFSTAT_TXMASK, \
+ .tx_fifoshift = S5PV210_UFSTAT_TXSHIFT, \
+ .def_clk_sel = S3C2410_UCON_CLKSEL0, \
+ .num_clks = 1, \
+ .clksel_mask = 0, \
+ .clksel_shift = 0, \
+ }, \
+ .def_cfg = &(struct s3c2410_uartcfg) { \
+ .ucon = S5PV210_UCON_DEFAULT, \
+ .ufcon = S5PV210_UFCON_DEFAULT, \
+ .has_fracval = 1, \
+ } \
+
static struct s3c24xx_serial_drv_data exynos4210_serial_drv_data = {
- .info = &(struct s3c24xx_uart_info) {
- .name = "Samsung Exynos4 UART",
- .type = PORT_S3C6400,
- .has_divslot = 1,
- .rx_fifomask = S5PV210_UFSTAT_RXMASK,
- .rx_fifoshift = S5PV210_UFSTAT_RXSHIFT,
- .rx_fifofull = S5PV210_UFSTAT_RXFULL,
- .tx_fifofull = S5PV210_UFSTAT_TXFULL,
- .tx_fifomask = S5PV210_UFSTAT_TXMASK,
- .tx_fifoshift = S5PV210_UFSTAT_TXSHIFT,
- .def_clk_sel = S3C2410_UCON_CLKSEL0,
- .num_clks = 1,
- .clksel_mask = 0,
- .clksel_shift = 0,
- },
- .def_cfg = &(struct s3c2410_uartcfg) {
- .ucon = S5PV210_UCON_DEFAULT,
- .ufcon = S5PV210_UFCON_DEFAULT,
- .has_fracval = 1,
- },
+ EXYNOS_COMMON_SERIAL_DRV_DATA,
.fifosize = { 256, 64, 16, 16 },
};
+
+static struct s3c24xx_serial_drv_data exynos5433_serial_drv_data = {
+ EXYNOS_COMMON_SERIAL_DRV_DATA,
+ .fifosize = { 64, 256, 16, 256 },
+};
+
#define EXYNOS4210_SERIAL_DRV_DATA ((kernel_ulong_t)&exynos4210_serial_drv_data)
+#define EXYNOS5433_SERIAL_DRV_DATA ((kernel_ulong_t)&exynos5433_serial_drv_data)
#else
#define EXYNOS4210_SERIAL_DRV_DATA (kernel_ulong_t)NULL
+#define EXYNOS5433_SERIAL_DRV_DATA (kernel_ulong_t)NULL
#endif
static struct platform_device_id s3c24xx_serial_driver_ids[] = {
@@ -1832,6 +1843,9 @@ static struct platform_device_id s3c24xx_serial_driver_ids[] = {
}, {
.name = "exynos4210-uart",
.driver_data = EXYNOS4210_SERIAL_DRV_DATA,
+ }, {
+ .name = "exynos5433-uart",
+ .driver_data = EXYNOS5433_SERIAL_DRV_DATA,
},
{ },
};
@@ -1851,6 +1865,8 @@ static const struct of_device_id s3c24xx_uart_dt_match[] = {
.data = (void *)S5PV210_SERIAL_DRV_DATA },
{ .compatible = "samsung,exynos4210-uart",
.data = (void *)EXYNOS4210_SERIAL_DRV_DATA },
+ { .compatible = "samsung,exynos5433-uart",
+ .data = (void *)EXYNOS5433_SERIAL_DRV_DATA },
{},
};
MODULE_DEVICE_TABLE(of, s3c24xx_uart_dt_match);
--
2.17.1
^ permalink raw reply related
* Re: [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-25 11:56 UTC (permalink / raw)
To: kbuild test robot
Cc: Sergey Senozhatsky, kbuild-all, linux-kernel, Petr Mladek,
Steven Rostedt, Daniel Wang, Peter Zijlstra, Andrew Morton,
Linus Torvalds, Greg Kroah-Hartman, Alan Cox, Jiri Slaby,
Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <201810251848.0wSJMnS0%fengguang.wu@intel.com>
On (10/25/18 18:51), kbuild test robot wrote:
>
> [auto build test ERROR on linux-sof-driver/master]
> [also build test ERROR on v4.19 next-20181019]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
My bad, sorry!
+#include <linux/vt_kern.h>
This should fix it.
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
From: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Subject: [PATCH] panic: avoid deadlocks in re-entrant console drivers
>From printk()/serial console point of view panic() is special, because
it may force CPU to re-enter printk() or/and serial console driver.
Therefore, some of serial consoles drivers are re-entrant. E.g. 8250:
serial8250_console_write()
{
if (port->sysrq)
locked = 0;
else if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
...
}
panic() does set oops_in_progress via bust_spinlocks(1), so in theory
we should be able to re-enter serial console driver from panic():
CPU0
<NMI>
uart_console_write()
serial8250_console_write() // if (oops_in_progress)
// spin_trylock_irqsave()
call_console_drivers()
console_unlock()
console_flush_on_panic()
bust_spinlocks(1) // oops_in_progress++
panic()
<NMI/>
spin_lock_irqsave(&port->lock, flags) // spin_lock_irqsave()
serial8250_console_write()
call_console_drivers()
console_unlock()
printk()
...
However, this does not happen and we deadlock in serial console on
port->lock spinlock. And the problem is that console_flush_on_panic()
called after bust_spinlocks(0):
void panic(const char *fmt, ...)
{
bust_spinlocks(1);
...
bust_spinlocks(0);
console_flush_on_panic();
...
}
bust_spinlocks(0) decrements oops_in_progress, so oops_in_progress
can go back to zero. Thus even re-entrant console drivers will simply
spin on port->lock spinlock. Given that port->lock may already be
locked either by a stopped CPU, or by the very same CPU we execute
panic() on (for instance, NMI panic() on printing CPU) the system
deadlocks and does not reboot.
Fix this by removing bust_spinlocks(0), so oops_in_progress is always
set in panic() now and, thus, re-entrant console drivers will trylock
the port->lock instead of spinning on it forever, when we call them
from console_flush_on_panic().
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
kernel/panic.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index f6d549a29a5c..272ac1c34e4b 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -29,6 +29,7 @@
#include <linux/ratelimit.h>
#include <linux/debugfs.h>
#include <asm/sections.h>
+#include <linux/vt_kern.h>
#define PANIC_TIMER_STEP 100
#define PANIC_BLINK_SPD 18
@@ -237,7 +238,10 @@ void panic(const char *fmt, ...)
if (_crash_kexec_post_notifiers)
__crash_kexec(NULL);
- bust_spinlocks(0);
+#ifdef CONFIG_VT
+ unblank_screen();
+#endif
+ console_unblank();
/*
* We may have ended up stopping the CPU holding the lock (in
--
2.19.1
^ permalink raw reply related
* Re: [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: kbuild test robot @ 2018-10-25 10:51 UTC (permalink / raw)
Cc: kbuild-all, linux-kernel, Petr Mladek, Steven Rostedt,
Daniel Wang, Peter Zijlstra, Andrew Morton, Linus Torvalds,
Greg Kroah-Hartman, Alan Cox, Jiri Slaby, Peter Feiner,
linux-serial, Sergey Senozhatsky, Sergey Senozhatsky
In-Reply-To: <20181025101036.6823-1-sergey.senozhatsky@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5410 bytes --]
Hi Sergey,
I love your patch! Yet something to improve:
[auto build test ERROR on linux-sof-driver/master]
[also build test ERROR on v4.19 next-20181019]
[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/Sergey-Senozhatsky/panic-avoid-deadlocks-in-re-entrant-console-drivers/20181025-181929
base: https://github.com/thesofproject/linux master
config: i386-randconfig-x077-201842 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/panic.c: In function 'panic':
>> kernel/panic.c:237:2: error: implicit declaration of function 'unblank_screen' [-Werror=implicit-function-declaration]
unblank_screen();
^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/unblank_screen +237 kernel/panic.c
186
187 /*
188 * If we have crashed and we have a crash kernel loaded let it handle
189 * everything else.
190 * If we want to run this after calling panic_notifiers, pass
191 * the "crash_kexec_post_notifiers" option to the kernel.
192 *
193 * Bypass the panic_cpu check and call __crash_kexec directly.
194 */
195 if (!_crash_kexec_post_notifiers) {
196 printk_safe_flush_on_panic();
197 __crash_kexec(NULL);
198
199 /*
200 * Note smp_send_stop is the usual smp shutdown function, which
201 * unfortunately means it may not be hardened to work in a
202 * panic situation.
203 */
204 smp_send_stop();
205 } else {
206 /*
207 * If we want to do crash dump after notifier calls and
208 * kmsg_dump, we will need architecture dependent extra
209 * works in addition to stopping other CPUs.
210 */
211 crash_smp_send_stop();
212 }
213
214 /*
215 * Run any panic handlers, including those that might need to
216 * add information to the kmsg dump output.
217 */
218 atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
219
220 /* Call flush even twice. It tries harder with a single online CPU */
221 printk_safe_flush_on_panic();
222 kmsg_dump(KMSG_DUMP_PANIC);
223
224 /*
225 * If you doubt kdump always works fine in any situation,
226 * "crash_kexec_post_notifiers" offers you a chance to run
227 * panic_notifiers and dumping kmsg before kdump.
228 * Note: since some panic_notifiers can make crashed kernel
229 * more unstable, it can increase risks of the kdump failure too.
230 *
231 * Bypass the panic_cpu check and call __crash_kexec directly.
232 */
233 if (_crash_kexec_post_notifiers)
234 __crash_kexec(NULL);
235
236 #ifdef CONFIG_VT
> 237 unblank_screen();
238 #endif
239 console_unblank();
240
241 /*
242 * We may have ended up stopping the CPU holding the lock (in
243 * smp_send_stop()) while still having some valuable data in the console
244 * buffer. Try to acquire the lock then release it regardless of the
245 * result. The release will also print the buffers out. Locks debug
246 * should be disabled to avoid reporting bad unlock balance when
247 * panic() is not being callled from OOPS.
248 */
249 debug_locks_off();
250 console_flush_on_panic();
251
252 if (!panic_blink)
253 panic_blink = no_blink;
254
255 if (panic_timeout > 0) {
256 /*
257 * Delay timeout seconds before rebooting the machine.
258 * We can't use the "normal" timers since we just panicked.
259 */
260 pr_emerg("Rebooting in %d seconds..\n", panic_timeout);
261
262 for (i = 0; i < panic_timeout * 1000; i += PANIC_TIMER_STEP) {
263 touch_nmi_watchdog();
264 if (i >= i_next) {
265 i += panic_blink(state ^= 1);
266 i_next = i + 3600 / PANIC_BLINK_SPD;
267 }
268 mdelay(PANIC_TIMER_STEP);
269 }
270 }
271 if (panic_timeout != 0) {
272 /*
273 * This will not be a clean reboot, with everything
274 * shutting down. But if there is a chance of
275 * rebooting the system it will be rebooted.
276 */
277 emergency_restart();
278 }
279 #ifdef __sparc__
280 {
281 extern int stop_a_enabled;
282 /* Make sure the user can actually press Stop-A (L1-A) */
283 stop_a_enabled = 1;
284 pr_emerg("Press Stop-A (L1-A) from sun keyboard or send break\n"
285 "twice on console to return to the boot prom\n");
286 }
287 #endif
288 #if defined(CONFIG_S390)
289 {
290 unsigned long caller;
291
292 caller = (unsigned long)__builtin_return_address(0);
293 disabled_wait(caller);
294 }
295 #endif
296 pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
297 local_irq_enable();
298 for (i = 0; ; i += PANIC_TIMER_STEP) {
299 touch_softlockup_watchdog();
300 if (i >= i_next) {
301 i += panic_blink(state ^= 1);
302 i_next = i + 3600 / PANIC_BLINK_SPD;
303 }
304 mdelay(PANIC_TIMER_STEP);
305 }
306 }
307
---
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: 33530 bytes --]
^ permalink raw reply
* [PATCHv3] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-25 10:10 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Mladek, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-2-sergey.senozhatsky@gmail.com>
>From printk()/serial console point of view panic() is special, because
it may force CPU to re-enter printk() or/and serial console driver.
Therefore, some of serial consoles drivers are re-entrant. E.g. 8250:
serial8250_console_write()
{
if (port->sysrq)
locked = 0;
else if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
...
}
panic() does set oops_in_progress via bust_spinlocks(1), so in theory
we should be able to re-enter serial console driver from panic():
CPU0
<NMI>
uart_console_write()
serial8250_console_write() // if (oops_in_progress)
// spin_trylock_irqsave()
call_console_drivers()
console_unlock()
console_flush_on_panic()
bust_spinlocks(1) // oops_in_progress++
panic()
<NMI/>
spin_lock_irqsave(&port->lock, flags) // spin_lock_irqsave()
serial8250_console_write()
call_console_drivers()
console_unlock()
printk()
...
However, this does not happen and we deadlock in serial console on
port->lock spinlock. And the problem is that console_flush_on_panic()
called after bust_spinlocks(0):
void panic(const char *fmt, ...)
{
bust_spinlocks(1);
...
bust_spinlocks(0);
console_flush_on_panic();
...
}
bust_spinlocks(0) decrements oops_in_progress, so oops_in_progress
can go back to zero. Thus even re-entrant console drivers will simply
spin on port->lock spinlock. Given that port->lock may already be
locked either by a stopped CPU, or by the very same CPU we execute
panic() on (for instance, NMI panic() on printing CPU) the system
deadlocks and does not reboot.
Fix this by removing bust_spinlocks(0), so oops_in_progress is always
set in panic() now and, thus, re-entrant console drivers will trylock
the port->lock instead of spinning on it forever, when we call them
from console_flush_on_panic().
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
v2: do not do bust_spinlocks(1);bust_spinlocks(0);bust_spinlocks(1)
thing and just copy paste from lib/bust_spinlocks what
bust_spinlocks(0) does. (Petr)
kernel/panic.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index f6d549a29a5c..fccd41628b24 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -237,7 +237,10 @@ void panic(const char *fmt, ...)
if (_crash_kexec_post_notifiers)
__crash_kexec(NULL);
- bust_spinlocks(0);
+#ifdef CONFIG_VT
+ unblank_screen();
+#endif
+ console_unblank();
/*
* We may have ended up stopping the CPU holding the lock (in
--
2.19.1
^ permalink raw reply related
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-25 9:31 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, linux-kernel, Steven Rostedt, Daniel Wang,
Peter Zijlstra, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20181025090603.7gkevv5l3zg7dj5e@pathway.suse.cz>
On (10/25/18 11:06), Petr Mladek wrote:
>
> IMHO, the custom s390 implementation can get removed.
> The generic code should do the same job these days.
>
Yep.
> > And console_unblank() is not guaranteed to print anything (unlike
> > console_flush_on_panic(), but oops is not panic() yet, so we can't
> > replace it with flush_on_panic()) - console_sem can be locked, so
> > console_unblank() would do nothing.
>
> I see. I missed that console_unblank() returns early when
> down_trylock_console_sem() fails.
>
> I still would like to refactor the code somehow to avoid
> the bust_spinlocks(0)/bust_spinlocks(1) ping-pong.
>
> It might make sense to call console_unblank() from
> console_flush_on_panic().
>
> I wonder if it would make sense to call unblank_screen() in
> console_unblank()...
These are interesting thoughts.
I can add one more thing to the list.
bust_spinlock() is probably not doing enough. It says that it
"clears any spinlocks which would prevent oops, die(), BUG()
and panic() information from reaching the user."
But this is, technically, not true. Because bust_spinlock() does not remove
console_sem out of sight. And we do have several spinlocks behind it. E.g.
semaphore's ->lock. Both down() and down_trylock() do
raw_spin_lock_irqsave(&sem->lock, flags), so if we got NMI panic while one
of the CPUs was under raw_spin_lock_irqsave(&sem->lock, flags) trying to
lock the console_sem then we are done. And IIRC we had exactly this type
of a bug report from LG 1 or 2 years ago - deadlock on sem->lock.
-ss
^ permalink raw reply
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Petr Mladek @ 2018-10-25 9:06 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181023121057.GC10251@jagdpanzerIV>
On Tue 2018-10-23 21:12:30, Sergey Senozhatsky wrote:
> On (10/23/18 21:04), Sergey Senozhatsky wrote:
> >
> > Seems that s390 is the only arch which defines its own bust_spinlocks().
> > Not sure why... Just to play games with console_loglevel?
> >
> > ---
> >
> > void bust_spinlocks(int yes)
> > {
> > if (yes) {
> > oops_in_progress = 1;
> > } else {
> > int loglevel_save = console_loglevel;
> > console_unblank();
> > oops_in_progress = 0;
> > /*
> > * OK, the message is on the console. Now we call printk()
> > * without oops_in_progress set so that printk will give klogd
> > * a poke. Hold onto your hats...
> > */
> > console_loglevel = 15;
> > printk(" ");
> > console_loglevel = loglevel_save;
> > }
> > }
> >
> > ---
> >
> > The "printk(" "); without oops_in_progress" part is a bit worrisome.
> > This thing technically can deadlock. Unless s390 has no NMI panic().
The comment suggests that the main purpose it is to wake up
klogd(). It is done in the generic bust_spinlocks() a safe way.
The games with console_loglevel do not make much sense because
console_unblank() calls console_unlock() when it was able to
take the console_lock. It means that either console_unblank()
flush all messages to the console or console_trylock() would
most likely fail also in that extra printk().
IMHO, the custom s390 implementation can get removed.
The generic code should do the same job these days.
> And console_unblank() is not guaranteed to print anything (unlike
> console_flush_on_panic(), but oops is not panic() yet, so we can't
> replace it with flush_on_panic()) - console_sem can be locked, so
> console_unblank() would do nothing.
I see. I missed that console_unblank() returns early when
down_trylock_console_sem() fails.
I still would like to refactor the code somehow to avoid
the bust_spinlocks(0)/bust_spinlocks(1) ping-pong.
It might make sense to call console_unblank() from
console_flush_on_panic().
I wonder if it would make sense to call unblank_screen() in
console_unblank()...
Best Regards,
Petr
^ permalink raw reply
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-25 9:05 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, linux-kernel, Steven Rostedt, Daniel Wang,
Peter Zijlstra, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20181025082947.gznv5roj3zxzlw3t@pathway.suse.cz>
On (10/25/18 10:29), Petr Mladek wrote:
>
> Yes, klogd is not a big deal. I just think that the bust_spinlocks()
> ping-pong would just confuse the code.
I agree; that's why I put some comments there.
> It might be better to keep the spinlocks busted and make sure that we do
> not cause regressions by not calling bust_spinlocks(0).
Sure, I understand. One reason to keep bust_spinlocks(0) there was "invoke
arch-specific bust_spinlocks(0), which might do something that common
bust_spinlocks() wouldn't do". Without going into details if any arch actually
does anything "special" in bust_spinlocks(0). Another reason was - this patch
looks like a -stable material to me; especially given that we have panic()
deadlock reports now. So I wanted to have a one liner which will not change
things for arch-s that re-define bust_spinlocks() and, at the same time,
fix the deadlock. Other than that I'm all for keeping spinlocks busted all
the time and just doing:
---
#ifdef CONFIG_VT
unblank_screen();
#endif
console_unblank();
---
in panic().
BTW, speaking of s390 bust_spinlocks(). It seems that starting from 4.21
all arch-s will use common bust_spinlocks() [1].
[..]
> > Hmm, I don't think I've seen any reports because of this. From printk/console
> > POV the locks which are not taken under oops_in_progress are not released.
>
> Fair enough. Let's keep debug_locks_off() in panic().
Agreed.
[1] lkml.kernel.org/r/20181025081108.GB26561@osiris
-ss
^ permalink raw reply
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Petr Mladek @ 2018-10-25 8:29 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181023115433.GA10251@jagdpanzerIV>
On Tue 2018-10-23 20:54:33, Sergey Senozhatsky wrote:
> On (10/23/18 13:07), Petr Mladek wrote:
> > Though this looks a bit weird.
> >
> > I have just realized that console_unblank() is called by
> > bust_spinlocks(0) and does basically the same as
> > console_flush_on_panic(). Also it does not make much
> > sense wake_up_klogd() there. Finally, it seems to be
> > too late to disable lockdep there.
>
> So I did look at what lib/bust_spinlocks.c does; and I agree that waking
> up klogd makes little sense, on the other hand it just sets per-cpu
> pending bit, so not a big deal. console_unlock() should do there the
> same thing as console_flush_on_panic().
Yes, klogd is not a big deal. I just think that the bust_spinlocks()
ping-pong would just confuse the code. It might be better to keep
the spinlocks busted and make sure that we do not cause regressions
by not calling bust_spinlocks(0).
It tried to solve this by calling the important actions from
bust_spinlocks(0) directly in panic(). I wanted to omit anything
that does not make sense in panic().
> > void __attribute__((weak)) bust_spinlocks(int yes)
> > {
> > if (yes) {
> > + /*
> > + * Some locks might get ignored in the Oops situation
> > + * to get an important work done. Locks debug should
> > + * be disabled to avoid reporting bad unlock balance.
> > + */
> > + debug_locks_off();
> > ++oops_in_progress;
>
> Hmm, I don't think I've seen any reports because of this. From printk/console
> POV the locks which are not taken under oops_in_progress are not released.
Fair enough. Let's keep debug_locks_off() in panic().
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: serial: add documentation for the SiFive UART driver
From: Rob Herring @ 2018-10-24 17:32 UTC (permalink / raw)
To: Paul Walmsley
Cc: open list:SERIAL DRIVERS, devicetree, linux-riscv,
linux-kernel@vger.kernel.org, Greg Kroah-Hartman, Mark Rutland,
Palmer Dabbelt, Paul Walmsley
In-Reply-To: <6571bb0e-b36a-1196-4d90-8aa62d8a2a90@sifive.com>
On Tue, Oct 23, 2018 at 10:05:40AM -0700, Paul Walmsley wrote:
>
> On 10/20/18 7:21 AM, Rob Herring wrote:
> > On Fri, Oct 19, 2018 at 5:06 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > > On 10/19/18 1:45 PM, Rob Herring wrote:
> > > > On Fri, Oct 19, 2018 at 1:48 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > > > > Add DT binding documentation for the Linux driver for the SiFive
> > > > > asynchronous serial IP block. Nothing too exotic.
> > > > >
> > > > > Cc: linux-serial@vger.kernel.org
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Cc: linux-riscv@lists.infradead.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Cc: Palmer Dabbelt <palmer@sifive.com>
> > > > > Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
> > > > > Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> > > > > Signed-off-by: Paul Walmsley <paul@pwsan.com>
> > > > > ---
> > > > > .../bindings/serial/sifive-serial.txt | 21 +++++++++++++++++++
> > > > > 1 file changed, 21 insertions(+)
> > > > > create mode 100644 Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/serial/sifive-serial.txt b/Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > > > new file mode 100644
> > > > > index 000000000000..8982338512f5
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > > > @@ -0,0 +1,21 @@
> > > > > +SiFive asynchronous serial interface (UART)
> > > > > +
> > > > > +Required properties:
> > > > > +
> > > > > +- compatible: should be "sifive,fu540-c000-uart0" or "sifive,uart0"
> > > > >
> > > > As I mentioned for the
> > > > intc and now the pwm block bindings, if you are going to do version
> > > > numbers please document the versioning scheme.
> > > >
> > > >
> > > > Will add that to the binding document.
> > I don't seem to be making my point clear. I don't want any of this
> > added to a binding doc for particular IP blocks. Write a common doc
> > that explains the scheme and addresses the questions I asked. Then
> > just reference that doc here.
> >
> > Maybe this is documented somewhere already? Otherwise, if one is
> > creating a new IP block, how do they know what the versioning scheme
> > is or what goes in the DT ROM?
>
>
> Seems like there might be some confusion between IP blocks as integrated on
> an SoC vs. IP blocks in isolation. It's not necessarily the SoC integrator
> that sets an IP block version number; this can come from the IP block vendor
> itself. So each IP block may have its own version numbering practices for
> the IP block alone.
>
>
> For SiFive IP blocks, we at SiFive could probably align on a common version
> numbering structure for what's in the sifive-blocks repository.
I thought you had that from what Palmer said and what I've seen so far.
You have at least 3 bindings so far it seems.
> But other IP blocks from other vendors may not align to that, or may not
> have version numbers exposed at all. In those cases there's no way for
> software folks to find out what they are, as you pointed out earlier. This
> is the case with most DT compatible strings in the kernel tree.
>
> For example, we've integrated the NVDLA IP block, from NVIDIA, on some
> designs. Any NVIDIA version numbers in that IP block will probably not
> follow the SiFive version numbering scheme. I'd propose the right thing to
> do for an IP block compatible string is to follow the vendor's practice, and
> then use the SoC integrator's version numbering practice for the
> SoC-integrated compatible string.
Experience has shown that using compatible strings only specific to
vendor IP blocks (with or without version numbers) is pretty useless.
For licensed IP, I'd suggest you follow standard practices. A genericish
fallback is generally only used when there's lots of SoCs sharing a
block.
In these cases though it needs to be clear what bindings follow some
common versioning scheme and which don't. That's accomplished
by referencing what the version scheme is. Otherwise, I'd expect I'll
see the versioning scheme copied when in fact the source IP in no way
follows it.
> In effect, an SoC integration DT compatible string like
> "sifive,fu540-c000-uart" implicitly states an IP block version number:
> "whatever came out of the fab on the chip"[**]. I'd propose that even in
> these cases, there's an advantage to keeping the "0" on the end, since it
> uniquely identifies an SoC-independent IP block, rather than just the type
> of the IP block. But if the "0" on the end of the SoC integration DT
> compatible string is problematic for you, we can certainly drop that last 0
> from the SoC integration DT compatible string, and only suffer a slight lack
> of clarity as to what version was integrated on that chip.
Personally I'd leave it off, but I'm fine with either way. It just needs
to be the way you document for SiFive IP blocks.
> But for IP block-specific version strings like "sifive,uart0", I think we
> can address your concern, at least for these public IP blocks. Since the
> SiFive UART and some other peripheral IP blocks are open-source, the public
> can have a pretty good idea of what DT version number corresponds to the
> source RTL, since the RTL is public. The version number identifies a
> specific programming model, without tying that programming model to any
> SoC-specific workarounds, etc. So for these cases, I think there's a pretty
> good case for having IP block-specific version numbers in DT compatible
> strings, and I hope you'll agree.
>
>
> The advantage for all of us is that there's then no need to embed
> chip-specific DT match strings in these drivers, for the most part. We just
> match on "sifive,uart0" and that's it, assuming no chip-specific workarounds
> are needed.
>
>
> > > > Where does the
> > > > number come from?
> > >
> > > It comes from the RTL, which is public:
> > >
> > > https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L43
> > I'm not going to go read your RTL, sorry.
>
>
> There's no need, but you did ask where it came from. Sorry you didn't like
> the answer.
I only meant that in context of reviewing the IP block. My questions
were meant to be what questions should a common document answer.
> Please let us know what you want us to do.
>
>
> Thanks for your review
>
>
> - Paul
>
>
> ** The caveat is that even with SoC identifiers in the Linux DT compatible
> strings, there's not enough information in many of the existing kernel DT
> compatibility strings to uniquely identify chip versions. Taking OMAP and
> Tegra as examples, there are several different chip versions for a given SoC
> generation that came out of the fab. OMAP chip version strings usually
> began with "ES"; Tegra version numbers, as I recall, were a letter and two
> numbers. For the most part, those versions were never specifically
> identified in the upstream kernel DT strings or in DT file names. (There are
> some exceptions with OMAP where we did identify specific chip version
> numbers, because sizable numbers of folks had boards with early silicon, and
> we were committed to supporting them at the time.) Sadly even adding
> these additional chip version identifiers to the DT strings wouldn't be
> perfect: I've seen at least one large vendor implementing metal-only ECOs
> without incrementing public chip version numbers. The point here is that
> we're already not uniquely identifying IP blocks with our current Linux DT
> compatibility string scheme.
Yes, I'm certainly aware of this aspect. We have to draw the line
somewhere between enough information to distinguish differences and
having a sane number of compatible strings. I mainly expect that the 1st
versions of SoCs are short lived and ECO changes don't affect
compatibility. That's obviously not always the case, but hopefully is
sufficient in most cases.
Really, I'd just like to see folks get better at putting version and
configuration information into registers. We only need DT for what we
can't discover.
Rob
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: serial: add documentation for the SiFive UART driver
From: Rob Herring @ 2018-10-24 16:53 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Paul Walmsley, linux-serial, devicetree, linux-riscv,
linux-kernel, Greg KH, mark.rutland, paul
In-Reply-To: <mhng-44b7601c-c2f4-4de8-a12e-7730e03d691e@palmer-si-x1c4>
On Mon, Oct 22, 2018 at 09:41:51AM -0700, Palmer Dabbelt wrote:
> On Fri, 19 Oct 2018 13:45:57 PDT (-0700), robh+dt@kernel.org wrote:
> > On Fri, Oct 19, 2018 at 1:48 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
> > >
> > > Add DT binding documentation for the Linux driver for the SiFive
> > > asynchronous serial IP block. Nothing too exotic.
> > >
> > > Cc: linux-serial@vger.kernel.org
> > > Cc: devicetree@vger.kernel.org
> > > Cc: linux-riscv@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Cc: Palmer Dabbelt <palmer@sifive.com>
> > > Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
> > > Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
> > > Signed-off-by: Paul Walmsley <paul@pwsan.com>
> > > ---
> > > .../bindings/serial/sifive-serial.txt | 21 +++++++++++++++++++
> > > 1 file changed, 21 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/serial/sifive-serial.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/serial/sifive-serial.txt b/Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > new file mode 100644
> > > index 000000000000..8982338512f5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/serial/sifive-serial.txt
> > > @@ -0,0 +1,21 @@
> > > +SiFive asynchronous serial interface (UART)
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: should be "sifive,fu540-c000-uart0" or "sifive,uart0"
> >
> > I assume once again, the last '0' is a version? As I mentioned for the
> > intc and now the pwm block bindings, if you are going to do version
> > numbers please document the versioning scheme. Palmer mentioned the
> > compatible string is part of the IP block repository? Where does the
> > number come from? What's the next version? Major vs. minor versions?
> > ECO fixes? Is the version s/w readable? How do you ensure it gets
> > updated? All that should be addressed.
>
> The RISC-V ecosystem is a bit different than that of ARM, MIPS, or Intel in
> that the ISA is an royalty-free open standard that anyone can implement (ie,
> without even signing a license agreement), with only the "RISC-V" trademark
> being held behind a pay+conformance wall. As a result, we don't actually
> have any control over who builds a RISC-V chip so all we at SiFive can
> really to is try to demonstrate good practices in software land and go from
> there.
Rights to the ISA and cores may be different, but how chips are built
is not really all that different (or doesn't have to be).
> As far as SiFive's codebase is concerned, the version number is embedded in
> the RTL generator, and a device tree is generated along with the RTL. This
> device tree is then embedded into a mask ROM on the chip, which allows the
> earliest stage of boot to proceed. As I'm sure you know, boot is a very
> complicated process and as a result the device tree passed to Linux doesn't
> necessarily look like what's in the ROM, but the intent is to keep iterating
> until we can get these as similar as possible -- that's why we're submitting
> every devicetree binding to the standard.
So all this discussion is purely SiFive specific and really has nothing
to do with RISC-V ecosystem.
Putting the DT into the ROM isn't something I'd do. It's simply not
going to work timeline wise IMO.
> Specifically as far as the UART is concerned, the compat string that's not
> chip-specific lives here (the "sifive,fu540-c000-uart" string lives in an
> internal chip repo that I can't point to):
>
> https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L43
>
> The version numbering scheme right now is pretty simple: I try to pay as
> much attention as possible to how the hardware changes (both by looking and
> with some automation), and I go yell at anyone who does something stupid. I
> know it's not the most scalable of schemes, but it's the best we have. The
> UART is actually an interesting case right now because we have an
> outstanding pull request that adds a bit to the UART and then adds
> "sifive,uart1" to the compat string
>
> https://github.com/sifive/sifive-blocks/pull/90
Relying on people to catch whether changes are important or not is bound
to fail. It's really got to be built into the design flow.
Even just updating a version register I've experienced the h/w designers
forgetting to update it.
> My intent is to ensure that the device tree's compat string uniquely
> identifies the software interface to a block. Thus, whenever a device's
> implementation changes in a software-visible way (bug fix or feature
> addition) we change the compat string -- either adding one (as is the case
> of the UART, where the compat string will be both "sifive,uart1" and
> "sifive,uart0" since the new feature is backwards compatible with the old
> software) or changing one (if the interface change is not compatible with
> old software).
What about config options? Say the UART has a configurable FIFO size.
What about major vs. minor version changes? Respins of chips would need
to make minor changes if picking up major changes are deemed too risky.
> Like I said above, this is all a manual process right now and this only
> applies to SiFive's implementations. I'm confident that I can at least
> ensure that, for any given SiFive implementation, a block's compat string
> will uniquely identify the software interface to it. For the rest of the
> RISC-V world all we can do is set a good example and review the software.
This is all good information and is essentially what I'm looking for. I
just don't want it lost in a reply to an email, but something you can
reference. Look at bindings/arm/primecell.txt for example. That
describes a family of IP blocks and not any specific device.
Whether the versioning is sufficient or not, I don't really care as long
as you docuemnt what it is so it is consistent. Since you have a common
schema across IP blocks, that means you should have a common document.
Rob
^ permalink raw reply
* Re: [PATCH V3 3/8] dt-bindings: mmc: fsl-imx-esdhc: add imx8qxp compatible string
From: Ulf Hansson @ 2018-10-24 10:59 UTC (permalink / raw)
To: A.s. Dong
Cc: Mark Rutland, devicetree@vger.kernel.org, dongas86@gmail.com,
catalin.marinas@arm.com, will.deacon@arm.com, Bough Chen,
robh+dt@kernel.org, dl-linux-imx, kernel@pengutronix.de,
linux-serial@vger.kernel.org, Fabio Estevam,
linux-mmc@vger.kernel.org, shawnguo@kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1539886483-15775-4-git-send-email-aisheng.dong@nxp.com>
On 18 October 2018 at 20:19, A.s. Dong <aisheng.dong@nxp.com> wrote:
> Add imx8qxp compatible string
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: linux-serial@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-mmc@vger.kernel.org
> Cc: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
> Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt | 1 +
Queued for 4.21, thanks!
Kind regards
Uffe
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> index 3e29050..9201a7d 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> +++ b/Documentation/devicetree/bindings/mmc/fsl-imx-esdhc.txt
> @@ -16,6 +16,7 @@ Required properties:
> "fsl,imx6sl-usdhc"
> "fsl,imx6sx-usdhc"
> "fsl,imx7d-usdhc"
> + "fsl,imx8qxp-usdhc"
>
> Optional properties:
> - fsl,wp-controller : Indicate to use controller internal write protection
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun
From: H. Peter Anvin @ 2018-10-23 20:14 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, linux-serial, Jiri Slaby, Al Viro,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Thomas Gleixner,
Kate Stewart, Philippe Ombredanne, Eugene Syromiatnikov,
linux-alpha, Alan Cox, stable
In-Reply-To: <DD4A5EC1-E0A8-4DB0-907F-F334C27CE54D@zytor.com>
On 10/23/18 09:02, hpa@zytor.com wrote:
>>
>> As I think Al's big termios cleanups are going to be hitting Linus's
>> tree soon, do you know how these patches interact with that?
>>
>> This patch seems like it will not, so I'll be glad to queue that up
>> after my first round of patches get merged to Linus later this week,
>> but
>> the second one worries me.
>>
>> thanks,
>>
>> greg k-h
>
> I have been working with Al; we had approached much the same problems but from different directions. Mine ended up being a bit more comprehensive as a result, so I think we're going to end up using my code with Al's reviews.
>
> So bottom line is that it should be all good.
>
[Al: Feel free to yell at me if I got that wrong.]
-hpa
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: serial: add documentation for the SiFive UART driver
From: Paul Walmsley @ 2018-10-23 17:05 UTC (permalink / raw)
To: Rob Herring
Cc: open list:SERIAL DRIVERS, devicetree, linux-riscv,
linux-kernel@vger.kernel.org, Greg Kroah-Hartman, Mark Rutland,
Palmer Dabbelt, Paul Walmsley
In-Reply-To: <CAL_JsqLagTgjDhZ02X=wPFDB4WF2bR7=LyzSW9D=ooo_XB_zOg@mail.gmail.com>
On 10/20/18 7:21 AM, Rob Herring wrote:
> On Fri, Oct 19, 2018 at 5:06 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>> On 10/19/18 1:45 PM, Rob Herring wrote:
>>> On Fri, Oct 19, 2018 at 1:48 PM Paul Walmsley <paul.walmsley@sifive.com> wrote:
>>>> Add DT binding documentation for the Linux driver for the SiFive
>>>> asynchronous serial IP block. Nothing too exotic.
>>>>
>>>> Cc: linux-serial@vger.kernel.org
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: linux-riscv@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Palmer Dabbelt <palmer@sifive.com>
>>>> Reviewed-by: Palmer Dabbelt <palmer@sifive.com>
>>>> Signed-off-by: Paul Walmsley <paul.walmsley@sifive.com>
>>>> Signed-off-by: Paul Walmsley <paul@pwsan.com>
>>>> ---
>>>> .../bindings/serial/sifive-serial.txt | 21 +++++++++++++++++++
>>>> 1 file changed, 21 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/serial/sifive-serial.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/serial/sifive-serial.txt b/Documentation/devicetree/bindings/serial/sifive-serial.txt
>>>> new file mode 100644
>>>> index 000000000000..8982338512f5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/serial/sifive-serial.txt
>>>> @@ -0,0 +1,21 @@
>>>> +SiFive asynchronous serial interface (UART)
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible: should be "sifive,fu540-c000-uart0" or "sifive,uart0"
>>>>
>>> As I mentioned for the
>>> intc and now the pwm block bindings, if you are going to do version
>>> numbers please document the versioning scheme.
>>>
>>>
>>> Will add that to the binding document.
> I don't seem to be making my point clear. I don't want any of this
> added to a binding doc for particular IP blocks. Write a common doc
> that explains the scheme and addresses the questions I asked. Then
> just reference that doc here.
>
> Maybe this is documented somewhere already? Otherwise, if one is
> creating a new IP block, how do they know what the versioning scheme
> is or what goes in the DT ROM?
Seems like there might be some confusion between IP blocks as integrated
on an SoC vs. IP blocks in isolation. It's not necessarily the SoC
integrator that sets an IP block version number; this can come from the
IP block vendor itself. So each IP block may have its own version
numbering practices for the IP block alone.
For SiFive IP blocks, we at SiFive could probably align on a common
version numbering structure for what's in the sifive-blocks repository.
But other IP blocks from other vendors may not align to that, or may not
have version numbers exposed at all. In those cases there's no way for
software folks to find out what they are, as you pointed out earlier.
This is the case with most DT compatible strings in the kernel tree.
For example, we've integrated the NVDLA IP block, from NVIDIA, on some
designs. Any NVIDIA version numbers in that IP block will probably not
follow the SiFive version numbering scheme. I'd propose the right thing
to do for an IP block compatible string is to follow the vendor's
practice, and then use the SoC integrator's version numbering practice
for the SoC-integrated compatible string.
In effect, an SoC integration DT compatible string like
"sifive,fu540-c000-uart" implicitly states an IP block version number:
"whatever came out of the fab on the chip"[**]. I'd propose that even
in these cases, there's an advantage to keeping the "0" on the end,
since it uniquely identifies an SoC-independent IP block, rather than
just the type of the IP block. But if the "0" on the end of the SoC
integration DT compatible string is problematic for you, we can
certainly drop that last 0 from the SoC integration DT compatible
string, and only suffer a slight lack of clarity as to what version was
integrated on that chip.
But for IP block-specific version strings like "sifive,uart0", I think
we can address your concern, at least for these public IP blocks. Since
the SiFive UART and some other peripheral IP blocks are open-source, the
public can have a pretty good idea of what DT version number corresponds
to the source RTL, since the RTL is public. The version number
identifies a specific programming model, without tying that programming
model to any SoC-specific workarounds, etc. So for these cases, I think
there's a pretty good case for having IP block-specific version numbers
in DT compatible strings, and I hope you'll agree.
The advantage for all of us is that there's then no need to embed
chip-specific DT match strings in these drivers, for the most part. We
just match on "sifive,uart0" and that's it, assuming no chip-specific
workarounds are needed.
>>> Where does the
>>> number come from?
>>
>> It comes from the RTL, which is public:
>>
>> https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/uart/UART.scala#L43
> I'm not going to go read your RTL, sorry.
There's no need, but you did ask where it came from. Sorry you didn't
like the answer.
Please let us know what you want us to do.
Thanks for your review
- Paul
** The caveat is that even with SoC identifiers in the Linux DT
compatible strings, there's not enough information in many of the
existing kernel DT compatibility strings to uniquely identify chip
versions. Taking OMAP and Tegra as examples, there are several
different chip versions for a given SoC generation that came out of the
fab. OMAP chip version strings usually began with "ES"; Tegra version
numbers, as I recall, were a letter and two numbers. For the most part,
those versions were never specifically identified in the upstream kernel
DT strings or in DT file names. (There are some exceptions with OMAP
where we did identify specific chip version numbers, because sizable
numbers of folks had boards with early silicon, and we were committed to
supporting them at the time.) Sadly even adding these additional chip
version identifiers to the DT strings wouldn't be perfect: I've seen at
least one large vendor implementing metal-only ECOs without incrementing
public chip version numbers. The point here is that we're already not
uniquely identifying IP blocks with our current Linux DT compatibility
string scheme.
^ permalink raw reply
* Re: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun
From: hpa @ 2018-10-23 16:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: linux-kernel, linux-serial, Jiri Slaby, Al Viro,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Thomas Gleixner,
Kate Stewart, Philippe Ombredanne, Eugene Syromiatnikov,
linux-alpha, Alan Cox, stable
In-Reply-To: <20181023145351.GA28516@kroah.com>
On October 23, 2018 7:53:51 AM PDT, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Mon, Oct 22, 2018 at 09:19:04AM -0700, H. Peter Anvin (Intel) wrote:
>> From: "H. Peter Anvin" <hpa@zytor.com>
>>
>> On architectures with CBAUDEX == 0 (Alpha and PowerPC), the code in
>tty_baudrate.c does
>> not do any limit checking on the tty_baudrate[] array, and in fact a
>> buffer overrun is possible on both architectures. Add a limit check
>to
>> prevent that situation.
>>
>> This will be followed by a much bigger cleanup/simplification patch.
>>
>> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>> Requested-by: Cc: Johan Hovold <johan@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Jiri Slaby <jslaby@suse.com>
>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
>> Cc: Matt Turner <mattst88@gmail.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Kate Stewart <kstewart@linuxfoundation.org>
>> Cc: Philippe Ombredanne <pombredanne@nexb.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Eugene Syromiatnikov <esyr@redhat.com>
>> Cc: <linux-alpha@vger.kernel.org>
>> Cc: <linux-serial@vger.kernel.org>
>> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
>> Cc: <stable@vger.kernel.org>
>> ---
>> drivers/tty/tty_baudrate.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>As I think Al's big termios cleanups are going to be hitting Linus's
>tree soon, do you know how these patches interact with that?
>
>This patch seems like it will not, so I'll be glad to queue that up
>after my first round of patches get merged to Linus later this week,
>but
>the second one worries me.
>
>thanks,
>
>greg k-h
I have been working with Al; we had approached much the same problems but from different directions. Mine ended up being a bit more comprehensive as a result, so I think we're going to end up using my code with Al's reviews.
So bottom line is that it should be all good.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH stable v2 1/2] termios, tty/tty_baudrate.c: fix buffer overrun
From: Greg Kroah-Hartman @ 2018-10-23 14:53 UTC (permalink / raw)
To: H. Peter Anvin (Intel)
Cc: linux-kernel, linux-serial, Jiri Slaby, Al Viro,
Richard Henderson, Ivan Kokshaysky, Matt Turner, Thomas Gleixner,
Kate Stewart, Philippe Ombredanne, Eugene Syromiatnikov,
linux-alpha, Alan Cox, stable
In-Reply-To: <20181022161905.26589-1-hpa@zytor.com>
On Mon, Oct 22, 2018 at 09:19:04AM -0700, H. Peter Anvin (Intel) wrote:
> From: "H. Peter Anvin" <hpa@zytor.com>
>
> On architectures with CBAUDEX == 0 (Alpha and PowerPC), the code in tty_baudrate.c does
> not do any limit checking on the tty_baudrate[] array, and in fact a
> buffer overrun is possible on both architectures. Add a limit check to
> prevent that situation.
>
> This will be followed by a much bigger cleanup/simplification patch.
>
> Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Requested-by: Cc: Johan Hovold <johan@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
> Cc: Matt Turner <mattst88@gmail.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kate Stewart <kstewart@linuxfoundation.org>
> Cc: Philippe Ombredanne <pombredanne@nexb.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Eugene Syromiatnikov <esyr@redhat.com>
> Cc: <linux-alpha@vger.kernel.org>
> Cc: <linux-serial@vger.kernel.org>
> Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Cc: <stable@vger.kernel.org>
> ---
> drivers/tty/tty_baudrate.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
As I think Al's big termios cleanups are going to be hitting Linus's
tree soon, do you know how these patches interact with that?
This patch seems like it will not, so I'll be glad to queue that up
after my first round of patches get merged to Linus later this week, but
the second one worries me.
thanks,
greg k-h
^ permalink raw reply
* [PATCH] pch_uart: remove set but not used variable 'tx_empty'
From: zhong jiang @ 2018-10-23 13:21 UTC (permalink / raw)
To: gregkh; +Cc: jslaby, linux-serial, linux-kernel
tx_empty is not used after setting its value. It is safe to remove
the unused variable.
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
drivers/tty/serial/pch_uart.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index cb85002..9ed121f 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -933,7 +933,6 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
struct scatterlist *sg;
int nent;
int fifo_size;
- int tx_empty;
struct dma_async_tx_descriptor *desc;
int num;
int i;
@@ -958,11 +957,9 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
}
fifo_size = max(priv->fifo_size, 1);
- tx_empty = 1;
if (pop_tx_x(priv, xmit->buf)) {
pch_uart_hal_write(priv, xmit->buf, 1);
port->icount.tx++;
- tx_empty = 0;
fifo_size--;
}
--
1.7.12.4
^ permalink raw reply related
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-23 12:12 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181023120441.GB10251@jagdpanzerIV>
On (10/23/18 21:04), Sergey Senozhatsky wrote:
>
> Seems that s390 is the only arch which defines its own bust_spinlocks().
> Not sure why... Just to play games with console_loglevel?
>
> ---
>
> void bust_spinlocks(int yes)
> {
> if (yes) {
> oops_in_progress = 1;
> } else {
> int loglevel_save = console_loglevel;
> console_unblank();
> oops_in_progress = 0;
> /*
> * OK, the message is on the console. Now we call printk()
> * without oops_in_progress set so that printk will give klogd
> * a poke. Hold onto your hats...
> */
> console_loglevel = 15;
> printk(" ");
> console_loglevel = loglevel_save;
> }
> }
>
> ---
>
> The "printk(" "); without oops_in_progress" part is a bit worrisome.
> This thing technically can deadlock. Unless s390 has no NMI panic().
And console_unblank() is not guaranteed to print anything (unlike
console_flush_on_panic(), but oops is not panic() yet, so we can't
replace it with flush_on_panic()) - console_sem can be locked, so
console_unblank() would do nothing.
That printk(" ") probably wants to be printk_deferred(" ").
-ss
^ permalink raw reply
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-23 12:04 UTC (permalink / raw)
To: Petr Mladek
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181023115433.GA10251@jagdpanzerIV>
On (10/23/18 20:54), Sergey Senozhatsky wrote:
> So I did look at what lib/bust_spinlocks.c does; and I agree that waking
> up klogd makes little sense, on the other hand it just sets per-cpu
> pending bit, so not a big deal. console_unlock() should do there the
> same thing as console_flush_on_panic(). Yes. However, a bit of a bigger
> argument:
> __attribute__((weak)) suggests that bust_spinlocks() is arch-dependent
> and it's up to arch to do some extra stuff there [if needed]. So that's
> why I decided to keep bust_spinlocks(0) in panic() and, thus, call into
> arch-specific code (or common bust_spinlocks); then bump oops_in_progress
> so serial consoles become re-entrant and finally call
> console_flush_on_panic().
Seems that s390 is the only arch which defines its own bust_spinlocks().
Not sure why... Just to play games with console_loglevel?
---
void bust_spinlocks(int yes)
{
if (yes) {
oops_in_progress = 1;
} else {
int loglevel_save = console_loglevel;
console_unblank();
oops_in_progress = 0;
/*
* OK, the message is on the console. Now we call printk()
* without oops_in_progress set so that printk will give klogd
* a poke. Hold onto your hats...
*/
console_loglevel = 15;
printk(" ");
console_loglevel = loglevel_save;
}
}
---
The "printk(" "); without oops_in_progress" part is a bit worrisome.
This thing technically can deadlock. Unless s390 has no NMI panic().
-ss
^ permalink raw reply
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-23 11:54 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, linux-kernel, Steven Rostedt, Daniel Wang,
Peter Zijlstra, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20181023110751.un2u67bc7dpo4ska@pathway.suse.cz>
On (10/23/18 13:07), Petr Mladek wrote:
> Though this looks a bit weird.
>
> I have just realized that console_unblank() is called by
> bust_spinlocks(0) and does basically the same as
> console_flush_on_panic(). Also it does not make much
> sense wake_up_klogd() there. Finally, it seems to be
> too late to disable lockdep there.
Thanks for taking a look.
As of "weird" part I have some explanations:
> @@ -233,17 +233,14 @@ void panic(const char *fmt, ...)
> if (_crash_kexec_post_notifiers)
> __crash_kexec(NULL);
>
> - bust_spinlocks(0);
> -
[..]
> - debug_locks_off();
> +#ifdef CONFIG_VT
> + unblank_screen();
> +#endif
> console_flush_on_panic();
>
> if (!panic_blink)
So I did look at what lib/bust_spinlocks.c does; and I agree that waking
up klogd makes little sense, on the other hand it just sets per-cpu
pending bit, so not a big deal. console_unlock() should do there the
same thing as console_flush_on_panic(). Yes. However, a bit of a bigger
argument:
__attribute__((weak)) suggests that bust_spinlocks() is arch-dependent
and it's up to arch to do some extra stuff there [if needed]. So that's
why I decided to keep bust_spinlocks(0) in panic() and, thus, call into
arch-specific code (or common bust_spinlocks); then bump oops_in_progress
so serial consoles become re-entrant and finally call
console_flush_on_panic().
> void __attribute__((weak)) bust_spinlocks(int yes)
> {
> if (yes) {
> + /*
> + * Some locks might get ignored in the Oops situation
> + * to get an important work done. Locks debug should
> + * be disabled to avoid reporting bad unlock balance.
> + */
> + debug_locks_off();
> ++oops_in_progress;
Hmm, I don't think I've seen any reports because of this. From printk/console
POV the locks which are not taken under oops_in_progress are not released.
Wrt to uart port we usually have "bool locked" flag and unlock
port->lock only if we locked it:
{
if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
...
if (locked)
spin_unlock_irqrestore(&port->lock, flags);
}
Wrt to console_sem we have
{
if (oops_in_progress)
if (!down_trylock_console_sem() != 0)
return;
...
console_unlock();
}
So the locks that we care about in this particular patch (console sem
and port->lock) probably should not see any locking imbalance.
If you have strong opinion then we can have debug_locks_off() change
as part of this patch. But maybe I'd prefer to have it as a separate
patch. What do you think?
-ss
^ permalink raw reply
* Re: [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Petr Mladek @ 2018-10-23 11:07 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-2-sergey.senozhatsky@gmail.com>
On Tue 2018-10-16 14:04:25, Sergey Senozhatsky wrote:
> >From printk()/serial console point of view panic() is special, because
> it may force CPU to re-enter printk() or/and serial console driver.
> Therefore, some of serial consoles drivers are re-entrant. E.g. 8250:
>
> serial8250_console_write()
> {
> if (port->sysrq)
> locked = 0;
> else if (oops_in_progress)
> locked = spin_trylock_irqsave(&port->lock, flags);
> else
> spin_lock_irqsave(&port->lock, flags);
> ...
> }
>
> panic() does set oops_in_progress via bust_spinlocks(1), so in theory
> we should be able to re-enter serial console driver from panic():
>
> CPU0
> <NMI>
> uart_console_write()
> serial8250_console_write() // if (oops_in_progress)
> // spin_trylock_irqsave()
> call_console_drivers()
> console_unlock()
> console_flush_on_panic()
> bust_spinlocks(1) // oops_in_progress++
> panic()
> <NMI/>
> spin_lock_irqsave(&port->lock, flags) // spin_lock_irqsave()
> serial8250_console_write()
> call_console_drivers()
> console_unlock()
> printk()
> ...
>
> However, this does not happen and we deadlock in serial console on
> port->lock spinlock. And the problem is that console_flush_on_panic()
> called after bust_spinlocks(0):
>
> void panic(const char *fmt, ...)
> {
> bust_spinlocks(1);
> ...
> bust_spinlocks(0);
> console_flush_on_panic();
> ...
> }
>
> bust_spinlocks(0) decrements oops_in_progress, so oops_in_progress
> can go back to zero. Thus even re-entrant console drivers will simply
> spin on port->lock spinlock. Given that port->lock may already be
> locked either by a stopped CPU, or by the very same CPU we execute
> panic() on (for instance, NMI panic() on printing CPU) the system
> deadlocks and does not reboot.
The idea makes sense to me. You are right that we already called
printk/console with busted spinlock many times in panic().
Therefore it should not be worse.
> Fix this by setting oops_in_progress before console_flush_on_panic(),
> so re-entrant console drivers will trylock the port->lock instead of
> spinning on it forever.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> kernel/panic.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index f6d549a29a5c..a0e60ccf3031 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -237,7 +237,13 @@ void panic(const char *fmt, ...)
> if (_crash_kexec_post_notifiers)
> __crash_kexec(NULL);
>
> + /*
> + * Decrement oops_in_progress and let bust_spinlocks() to
> + * unblank_screen(), console_unblank() and wake_up_klogd()
> + */
> bust_spinlocks(0);
> + /* Set oops_in_progress, so we can reenter serial console driver */
> + bust_spinlocks(1);
Though this looks a bit weird.
I have just realized that console_unblank() is called by
bust_spinlocks(0) and does basically the same as
console_flush_on_panic(). Also it does not make much
sense wake_up_klogd() there. Finally, it seems to be
too late to disable lockdep there.
I would suggest something like:
diff --git a/kernel/panic.c b/kernel/panic.c
index 8b2e002d52eb..c78e3df8dd58 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -233,17 +233,14 @@ void panic(const char *fmt, ...)
if (_crash_kexec_post_notifiers)
__crash_kexec(NULL);
- bust_spinlocks(0);
-
/*
* We may have ended up stopping the CPU holding the lock (in
- * smp_send_stop()) while still having some valuable data in the console
- * buffer. Try to acquire the lock then release it regardless of the
- * result. The release will also print the buffers out. Locks debug
- * should be disabled to avoid reporting bad unlock balance when
- * panic() is not being callled from OOPS.
+ * smp_send_stop()) while still having some valuable data in
+ * the console buffer. Try hard to see them.
*/
- debug_locks_off();
+#ifdef CONFIG_VT
+ unblank_screen();
+#endif
console_flush_on_panic();
if (!panic_blink)
diff --git a/lib/bust_spinlocks.c b/lib/bust_spinlocks.c
index ab719495e2cb..e42d2fcd6453 100644
--- a/lib/bust_spinlocks.c
+++ b/lib/bust_spinlocks.c
@@ -20,6 +20,12 @@
void __attribute__((weak)) bust_spinlocks(int yes)
{
if (yes) {
+ /*
+ * Some locks might get ignored in the Oops situation
+ * to get an important work done. Locks debug should
+ * be disabled to avoid reporting bad unlock balance.
+ */
+ debug_locks_off();
++oops_in_progress;
} else {
#ifdef CONFIG_VT
^ permalink raw reply related
* Re: [RFC][PATCHv2 2/4] printk: move printk_safe macros to printk header
From: Sergey Senozhatsky @ 2018-10-23 6:25 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Sergey Senozhatsky, linux-kernel, Petr Mladek, Steven Rostedt,
Daniel Wang, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20181016125415.GA3121@hirez.programming.kicks-ass.net>
On (10/16/18 14:54), Peter Zijlstra wrote:
>
> No, no wakups. irq_work to wake the printk-thread, at most.
>
There are cases when we probably prefer to be in "direct printk" mode.
E.g. sysrq or late PM stages (probably).
Doing irq_work->wake_up_process->printk_kthread from sysrq probably
might not work all the time, so direct printk path may look more
"reliable" in some cases. In *theory* (just in theory), we can do
void __handle_sysrq(int key, bool check_mask)
{
...
op_p->handler(key);
...
+ if (console_trylock())
+ console_unlock();
}
type of thing. So sysrq handler will just log_store() the data and we
will try to flush logbuf immediately once we are dont with sysrq handler.
This will require additional work, tho. Some sysrq handlers can print
significant (depends on serial console baud) amounts of data. For instance,
sysrq_handle_showstate() calls show_workqueue_state() and show_state(),
which do numerous printk()-s. Therefore, those functions touch NMI and
softlockup watchdogs:
void show_workqueue_state(void)
{
for_each_pwq(pwq, wq) {
...
touch_nmi_watchdog();
}
for_each_pool(pool, pi) {
...
touch_nmi_watchdog();
}
}
and
void show_state_filter(unsigned long state_filter)
{
for_each_process_thread(g, p) {
touch_nmi_watchdog();
touch_all_softlockup_watchdogs();
...
}
}
If we will move the actual printout to console_unlock() then we will
have to start touching watchdogs from console_unlock().
Another troubling moment might be that with completely async printk()
it's easier to cause logbuf wrap around; because CPU which does printk()
in a loop is always async, console drivers don't throttle it anymore;
currently it's sometimes sync (when console_sem is not locked, or locked
but we have active console_sem_owner) or async (when console_sem is
locked and there is no active console_sem_owner).
-ss
^ 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