| Message ID | 20181030221107.79758-1-dianders@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | serial: Finish sysrq on qcom_geni; fix sysrq vs. lockdep on 8250 | expand |
On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote: > I started out this series trying to make sysrq work over the serial > console on qcom_geni_serial, then fell into a rat's nest. > > To solve the deadlock I faced when enabling sysrq I tried to borrow > code from '8250_port.c' which avoided grabbing the port lock in > console_write(). ...but since these days I try to run with lockdep on > all the time, I found it caused an annoying lockdep splat (which I > also reproduced on my rk3399 board). ...so I instead changed my > qcom_geni_serial solution to borrow code from 'msm_serial.c' > > I wasn't super happy with the solution in 'msm_serial.c' though. I > don't like releasing the spinlock there. Not only is it ugly but it > means we are unlocking / re-locking _all the time_ even though sysrq > characters are rare. ...so I came up with what I think is a better > solution and then implemented it for qcom_geni_serial. > > Since I had a good way to test 8250-based UARTs, I also fixed that > driver to use my new method. When doing so, I ran into a missing > include in serial_core.h. NOTE: I didn't have a way to test > msm_serial.c at all, so I didn't switch that (or all other serial > drivers for that matter) to the new method. > > NOTE: from a serial point of view v2 is the same as v1 but I've > removed the extra kgdb-related patches and made it obvious that this > is really for all sysrq, not just kgdb. I've also generally tried to > curate the CCs more properly. It seems your forgot console people to Cc. > > > Douglas Anderson (5): > serial: qcom_geni_serial: Finish supporting sysrq > serial: core: Allow processing sysrq at port unlock time > serial: qcom_geni_serial: Process sysrq at port unlock time > serial: core: Include console.h from serial_core.h > serial: 8250: Process sysrq at port unlock time > > drivers/tty/serial/8250/8250_aspeed_vuart.c | 6 +++- > drivers/tty/serial/8250/8250_fsl.c | 6 +++- > drivers/tty/serial/8250/8250_omap.c | 6 +++- > drivers/tty/serial/8250/8250_port.c | 8 ++--- > drivers/tty/serial/qcom_geni_serial.c | 10 ++++-- > include/linux/serial_core.h | 38 ++++++++++++++++++++- > 6 files changed, 63 insertions(+), 11 deletions(-) > > -- > 2.19.1.568.g152ad8e336-goog >
Hi, On Wed, Nov 7, 2018 at 10:23 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote: > > I started out this series trying to make sysrq work over the serial > > console on qcom_geni_serial, then fell into a rat's nest. > > > > To solve the deadlock I faced when enabling sysrq I tried to borrow > > code from '8250_port.c' which avoided grabbing the port lock in > > console_write(). ...but since these days I try to run with lockdep on > > all the time, I found it caused an annoying lockdep splat (which I > > also reproduced on my rk3399 board). ...so I instead changed my > > qcom_geni_serial solution to borrow code from 'msm_serial.c' > > > > I wasn't super happy with the solution in 'msm_serial.c' though. I > > don't like releasing the spinlock there. Not only is it ugly but it > > means we are unlocking / re-locking _all the time_ even though sysrq > > characters are rare. ...so I came up with what I think is a better > > solution and then implemented it for qcom_geni_serial. > > > > Since I had a good way to test 8250-based UARTs, I also fixed that > > driver to use my new method. When doing so, I ran into a missing > > include in serial_core.h. NOTE: I didn't have a way to test > > msm_serial.c at all, so I didn't switch that (or all other serial > > drivers for that matter) to the new method. > > > > NOTE: from a serial point of view v2 is the same as v1 but I've > > removed the extra kgdb-related patches and made it obvious that this > > is really for all sysrq, not just kgdb. I've also generally tried to > > curate the CCs more properly. > > It seems your forgot console people to Cc. Can you be more specific, please? Which section of the "MAINTAINERS" file should I be looking at for the "console" you are thinking of? Certainly there are lots of hits for "console" in MAINTAINERS but I don't think I see any that are relevant that I missed. Grepping: $ grep -i console MAINTAINERS HYPERVISOR VIRTUAL CONSOLE DRIVER F: drivers/video/console/sti* PCDP - PRIMARY CONSOLE AND DEBUG PORT SGI SN-IA64 (Altix) SERIAL CONSOLE DRIVER STAGING - SPEAKUP CONSOLE SPEECH DRIVER VIRTIO CONSOLE DRIVER F: drivers/char/virtio_console.c F: include/linux/virtio_console.h F: include/uapi/linux/virtio_console.h ...none of those seem relevant upon first glance but I'm happy to stand corrected. Ah! Based on who you added to the CC list I guess you meant to CC "printk" folks? PRINTK M: Petr Mladek <pmladek@suse.com> M: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> R: Steven Rostedt <rostedt@goodmis.org> S: Maintained F: kernel/printk/ F: include/linux/printk.h I'd be happy to CC those folks on future spins (if there are any). I'm not convinced that these patches are directly relevant to the printk subsystem, but I'm always happy for more people to have a chance to review patches. Hopefully anyone who needs this patch can find it on one of the relevant mailing lists. I screwed up and missed LKML this time around, but there are plenty of other mailing lists here that it could be found on. If requested I'm also happy to re-post the same series adding those 3 people if that's what everyone wants. -Doug
On Wed, Nov 07, 2018 at 11:26:56AM -0800, Doug Anderson wrote: > Hi, > > On Wed, Nov 7, 2018 at 10:23 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote: > > > I started out this series trying to make sysrq work over the serial > > > console on qcom_geni_serial, then fell into a rat's nest. > > > > > > To solve the deadlock I faced when enabling sysrq I tried to borrow > > > code from '8250_port.c' which avoided grabbing the port lock in > > > console_write(). ...but since these days I try to run with lockdep on > > > all the time, I found it caused an annoying lockdep splat (which I > > > also reproduced on my rk3399 board). ...so I instead changed my > > > qcom_geni_serial solution to borrow code from 'msm_serial.c' > > > > > > I wasn't super happy with the solution in 'msm_serial.c' though. I > > > don't like releasing the spinlock there. Not only is it ugly but it > > > means we are unlocking / re-locking _all the time_ even though sysrq > > > characters are rare. ...so I came up with what I think is a better > > > solution and then implemented it for qcom_geni_serial. > > > > > > Since I had a good way to test 8250-based UARTs, I also fixed that > > > driver to use my new method. When doing so, I ran into a missing > > > include in serial_core.h. NOTE: I didn't have a way to test > > > msm_serial.c at all, so I didn't switch that (or all other serial > > > drivers for that matter) to the new method. > > > > > > NOTE: from a serial point of view v2 is the same as v1 but I've > > > removed the extra kgdb-related patches and made it obvious that this > > > is really for all sysrq, not just kgdb. I've also generally tried to > > > curate the CCs more properly. > > > > It seems your forgot console people to Cc. > > Can you be more specific, please? Which section of the "MAINTAINERS" > file should I be looking at for the "console" you are thinking of? I have added them to Cc list: Petr, Sergey, and Steven. > Certainly there are lots of hits for "console" in MAINTAINERS but I > don't think I see any that are relevant that I missed. Grepping: > Ah! Based on who you added to the CC list I guess you meant to CC > "printk" folks? Correct. > PRINTK > M: Petr Mladek <pmladek@suse.com> > M: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > R: Steven Rostedt <rostedt@goodmis.org> > S: Maintained > F: kernel/printk/ > F: include/linux/printk.h > I'd be happy to CC those folks on future spins (if there are any). > I'm not convinced that these patches are directly relevant to the > printk subsystem, but I'm always happy for more people to have a > chance to review patches. If you look retrospectively in the mailing lists, you can find that they are doing most of the console core work, which I believe includes SysRq behaviour. So, don't be confused their names are listed under PRINTK. > Hopefully anyone who needs this patch can find it on one of the > relevant mailing lists. I screwed up and missed LKML this time > around, but there are plenty of other mailing lists here that it could > be found on. If requested I'm also happy to re-post the same series > adding those 3 people if that's what everyone wants.
On Wed 2018-11-07 11:26:56, Doug Anderson wrote: > On Wed, Nov 7, 2018 at 10:23 AM Andy Shevchenko > Ah! Based on who you added to the CC list I guess you meant to CC > "printk" folks? > > PRINTK > M: Petr Mladek <pmladek@suse.com> > M: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > R: Steven Rostedt <rostedt@goodmis.org> > S: Maintained > F: kernel/printk/ > F: include/linux/printk.h > > > I'd be happy to CC those folks on future spins (if there are any). > I'm not convinced that these patches are directly relevant to the > printk subsystem, but I'm always happy for more people to have a > chance to review patches. I, as a printk maintainer, am not completely familiar with all the console driver problems. But we, printk maintainers, come to similar deadlocks from the printk side, so we are definitely interested into this kind of patches. BTW: There was an attempt to avoid the console_unlock() related deadlocks a more generic way, see https://lore.kernel.org/lkml/20181016050428.17966-1-sergey.senozhatsky@gmail.com Unfortunately, there is some push back against introducing a new printk-related-locking API. > Hopefully anyone who needs this patch can find it on one of the > relevant mailing lists. I screwed up and missed LKML this time > around, but there are plenty of other mailing lists here that it could > be found on. If requested I'm also happy to re-post the same series > adding those 3 people if that's what everyone wants. I have glanced over the patches via https://www.spinics.net/lists/linux-arm-msm/msg44083.html I still have to think about it. I will be traveling next week so it might take some time. Anyway, please CC printk people into v2 if any. Best Reagards, Petr
On Tue, Oct 30, 2018 at 03:11:02PM -0700, Douglas Anderson wrote: > I started out this series trying to make sysrq work over the serial > console on qcom_geni_serial, then fell into a rat's nest. > > To solve the deadlock I faced when enabling sysrq I tried to borrow > code from '8250_port.c' which avoided grabbing the port lock in > console_write(). ...but since these days I try to run with lockdep on > all the time, I found it caused an annoying lockdep splat (which I > also reproduced on my rk3399 board). ...so I instead changed my > qcom_geni_serial solution to borrow code from 'msm_serial.c' > > I wasn't super happy with the solution in 'msm_serial.c' though. I > don't like releasing the spinlock there. Not only is it ugly but it > means we are unlocking / re-locking _all the time_ even though sysrq > characters are rare. ...so I came up with what I think is a better > solution and then implemented it for qcom_geni_serial. > > Since I had a good way to test 8250-based UARTs, I also fixed that > driver to use my new method. When doing so, I ran into a missing > include in serial_core.h. NOTE: I didn't have a way to test > msm_serial.c at all, so I didn't switch that (or all other serial > drivers for that matter) to the new method. > > NOTE: from a serial point of view v2 is the same as v1 but I've > removed the extra kgdb-related patches and made it obvious that this > is really for all sysrq, not just kgdb. I've also generally tried to > curate the CCs more properly. Looks good, thanks for cleaning this up. All now queued up. greg k-h