Hi Geert,
On Mon, Feb 06, 2017 at 03:38:02PM +0100, Geert Uytterhoeven wrote:
> Hi all,
>
> Character LCDs are currently typically driven from userspace, either
> directly through bit-banging, or via some kind of serial (UART/i2c)
> interface.
>
> This patch series adds kernel support for character LCDs using an
> Hitachi HD44780 Character LCD Controller where its M6800 bus interface
> is connected to GPIOs. It does so after extracting the character LCD
> core support from the existing Parallel port LCD/Keypad Panel driver
> into its own subdriver.
>
> - Patch 1 fixes a bug in the existing panel driver,
> - Patches 2-8 perform clean ups and improvements to the panel driver,
> - Patch 9 extract the character LCD core support from the panel driver
> into an auxdisplay charlcd subdriver,
> - Patches 10-11 add new features to the character LCD core driver
> (4-bit interface support and support for displays with more than 2
> lines),
> - Patch 12 adds DT bindings for describing HD44780 Character LCDs,
> - Patch 13 adds a driver for HD44780 Character LCDs connected to
> GPIOs, using the new character LCD core.
>
> Note that Linux has other support for HD44780 Character LCDs, but none of
> it is generic:
> - drivers/misc/arm-charlcd.c involves a custom IP block for interfacing
> to the LCD controller,
> - drivers/parisc/led.c is tightly coupled to LED handling on various HP
> PA-RISC machines.
>
> This has been tested using a 20x4 character LCD with backlight, using both
> 8-bit and 4-bit wiring to one or two 74HC595 shift registers connected to
> an SPI master controller.
>
> Regression testing on original "panel" hardware would be appreciated.
So I finally had the time to give it a try by applying your patchset
on top of our 4.9 prod kernel. For now I'm getting a panic at boot :
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffff813e203d>] __mutex_lock_slowpath+0x6d/0x100
PGD 0
Oops: 0002 [#1] SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-alb90.0-alb3100-lcdtest #3
Hardware name: To Be Filled By Intel To Be Filled By Intel/To be filled by Intel, BIOS 080015 04/01/2011
task: ffff88007c488000 task.stack: ffff88007c490000
RIP: 0010:[<ffffffff813e203d>] [<ffffffff813e203d>] __mutex_lock_slowpath+0x6d/0x100
RSP: 0000:ffff88007c493cc8 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff88007c5a0f90 RCX: ffff88007c48d700
RDX: ffff88007c493cd0 RSI: 0000000000000000 RDI: ffff88007c5a0f94
RBP: ffff88007c488000 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000001 R11: 0000000002ea48c3 R12: ffff88007c5a0f94
R13: 00000000ffffffff R14: ffff88007c5a0f98 R15: ffff88007c470000
FS: 0000000000000000(0000) GS:ffff88007ee00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000001607000 CR4: 00000000000006f0
Stack:
ffff88007ee0ea80 ffff88007c5a0f98 0000000000000000 0000000000000282
ffff88007c5a0f90 ffff88007c5a0f90 0000000000000000 ffff88007c493f28
ffff88007c493f28 ffffffff813e20de ffff88007c5a0f00 ffffffff812a9cc3
Call Trace:
[<ffffffff813e20de>] ? mutex_lock+0xe/0x20
[<ffffffff812a9cc3>] ? charlcd_backlight+0x33/0x80
[<ffffffff812696c0>] ? lcd_write_data_p8+0x80/0x80
[<ffffffff812a9fa5>] ? charlcd_init_display+0x135/0x1a0
[<ffffffff812aaa6f>] ? charlcd_register+0xf/0xf0
[<ffffffff81269e48>] ? panel_attach+0x4c8/0x9c0
[<ffffffff8124e6b0>] ? attach_driver_chain+0x60/0x60
[<ffffffff8124e6cb>] ? port_check+0x1b/0x30
[<ffffffff812565af>] ? bus_for_each_dev+0x4f/0x80
[<ffffffff8124e842>] ? __parport_register_driver+0x112/0x150
[<ffffffff8169744a>] ? loop_init+0x11d/0x13c
[<ffffffff81697469>] ? loop_init+0x13c/0x13c
[<ffffffff81697622>] ? panel_init_module+0x1b9/0x20a
[<ffffffff81000412>] ? do_one_initcall+0x42/0x160
[<ffffffff81675124>] ? kernel_init_freeable+0x10e/0x1d8
[<ffffffff8167483d>] ? loglevel+0x36/0x36
[<ffffffff813ded10>] ? rest_init+0x70/0x70
[<ffffffff813ded1a>] ? kernel_init+0xa/0x100
[<ffffffff813ded10>] ? rest_init+0x70/0x70
[<ffffffff813e4212>] ? ret_from_fork+0x22/0x30
Code: 03 83 f8 01 0f 84 9d 00 00 00 48 8b 43 10 48 8d 54 24 08 4c 8d 73 08 48 89 53 10 4c 89 74 24 08 48 89 44 24 10 41 bd ff ff ff ff <48> 89 10 48 89 6c 24 18 eb 1a 48 c7 45 08 02 00 00 00 c6 43 04
RIP [<ffffffff813e203d>] __mutex_lock_slowpath+0x6d/0x100
RSP <ffff88007c493cc8>
CR2: 0000000000000000
---[ end trace e58465f0f36b5a27 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009
I suspect that's related to the backlight timer though I'm not sure.
In case that helps I'm using the following config :
$ grep 'PANEL\|LCD' .config
CONFIG_PANEL=y
CONFIG_PANEL_PARPORT=0
CONFIG_PANEL_PROFILE=4
CONFIG_PANEL_CHANGE_MESSAGE=y
CONFIG_PANEL_BOOT_MESSAGE="ALOHA ALB3100"
# CONFIG_BACKLIGHT_LCD_SUPPORT is not set
# CONFIG_HID_PICOLCD is not set
# CONFIG_USB_LCD is not set
CONFIG_CHARLCD=y
I'm going to comment out the backlight code to see if it improves
the situation.
Cheers,
Willy
Hi again,
On Thu, Mar 23, 2017 at 06:15:01PM +0100, Willy TARREAU wrote:
> So I finally had the time to give it a try by applying your patchset
> on top of our 4.9 prod kernel. For now I'm getting a panic at boot :
>
> BUG: unable to handle kernel NULL pointer dereference at (null)
> IP: [<ffffffff813e203d>] __mutex_lock_slowpath+0x6d/0x100
(...)
It was indeed related to the backlight mutex. It's initialized
after charlcd_init_display() succeeds, but the backlight is
touched within this function. I just moved it upwards and that
solved the issue. Feel free to re-integrate this (or an equivalent)
into your patch.
With only this fix, everything works fine here without even
having to touch the configuration, good job!
Thanks!
Willy
--
diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
index 6aea900..e7f1bfe 100644
--- a/drivers/auxdisplay/charlcd.c
+++ b/drivers/auxdisplay/charlcd.c
@@ -703,6 +703,8 @@ static int charlcd_init(struct charlcd *lcd)
struct charlcd_priv *priv = to_priv(lcd);
int ret;
+ mutex_init(&priv->bl_tempo_lock);
+
/*
* before this line, we must NOT send anything to the display.
* Since charlcd_init_display() needs to write data, we have to
@@ -713,7 +715,6 @@ static int charlcd_init(struct charlcd *lcd)
return ret;
if (lcd->ops->backlight) {
- mutex_init(&priv->bl_tempo_lock);
INIT_DELAYED_WORK(&priv->bl_work, charlcd_bl_off);
}
Hi Willy,
On Thu, Mar 23, 2017 at 6:37 PM, Willy TARREAU <[email protected]> wrote:
> On Thu, Mar 23, 2017 at 06:15:01PM +0100, Willy TARREAU wrote:
>> So I finally had the time to give it a try by applying your patchset
>> on top of our 4.9 prod kernel. For now I'm getting a panic at boot :
>>
>> BUG: unable to handle kernel NULL pointer dereference at (null)
>> IP: [<ffffffff813e203d>] __mutex_lock_slowpath+0x6d/0x100
> (...)
>
> It was indeed related to the backlight mutex. It's initialized
> after charlcd_init_display() succeeds, but the backlight is
> touched within this function. I just moved it upwards and that
> solved the issue. Feel free to re-integrate this (or an equivalent)
> into your patch.
>
> With only this fix, everything works fine here without even
> having to touch the configuration, good job!
Thanks for testing!
>
> Thanks!
> Willy
> --
>
> diff --git a/drivers/auxdisplay/charlcd.c b/drivers/auxdisplay/charlcd.c
> index 6aea900..e7f1bfe 100644
> --- a/drivers/auxdisplay/charlcd.c
> +++ b/drivers/auxdisplay/charlcd.c
> @@ -703,6 +703,8 @@ static int charlcd_init(struct charlcd *lcd)
> struct charlcd_priv *priv = to_priv(lcd);
> int ret;
>
> + mutex_init(&priv->bl_tempo_lock);
> +
> /*
> * before this line, we must NOT send anything to the display.
> * Since charlcd_init_display() needs to write data, we have to
> @@ -713,7 +715,6 @@ static int charlcd_init(struct charlcd *lcd)
> return ret;
>
> if (lcd->ops->backlight) {
> - mutex_init(&priv->bl_tempo_lock);
> INIT_DELAYED_WORK(&priv->bl_work, charlcd_bl_off);
> }
I take it you're using v1 of the patchset?
v2 had this change:
- Move backlight mutex initialization before call to
charlcd_init_display(), to prevent locking an uninitialized mutex
(reported by 0day).
https://lkml.org/lkml/2017/3/10/1151
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Thu, Mar 23, 2017 at 07:49:53PM +0100, Geert Uytterhoeven wrote:
> I take it you're using v1 of the patchset?
>
> v2 had this change:
>
> - Move backlight mutex initialization before call to
> charlcd_init_display(), to prevent locking an uninitialized mutex
> (reported by 0day).
>
> https://lkml.org/lkml/2017/3/10/1151
Ah yes good point sorry about that. I was surprized that the patches I
applied were that old as I remembered reading about them not that long
ago :-) I have too much stuff in this mailbox...
Given the few differences, I don't think I need to re-test it. But
just let me know if you need another test.
Cheers,
Willy
Hi Willy,
On Thu, Mar 23, 2017 at 8:03 PM, Willy TARREAU <[email protected]> wrote:
> On Thu, Mar 23, 2017 at 07:49:53PM +0100, Geert Uytterhoeven wrote:
>> I take it you're using v1 of the patchset?
>>
>> v2 had this change:
>>
>> - Move backlight mutex initialization before call to
>> charlcd_init_display(), to prevent locking an uninitialized mutex
>> (reported by 0day).
>>
>> https://lkml.org/lkml/2017/3/10/1151
>
> Ah yes good point sorry about that. I was surprized that the patches I
> applied were that old as I remembered reading about them not that long
> ago :-) I have too much stuff in this mailbox...
>
> Given the few differences, I don't think I need to re-test it. But
> just let me know if you need another test.
I don't think there's a need to re-test.
I did manage to reproduce the original problem when 0day reported it,
and verified that it was fixed.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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