Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1607738 > unrolled thread

Re: [PATCH 00/13] Add HD44780 Character LCD support

Started byWilly TARREAU <wtarreau@haproxy.com>
First post2017-03-23 18:40 +0100
Last post2017-03-23 21:30 +0100
Articles 5 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 00/13] Add HD44780 Character LCD support Willy TARREAU <wtarreau@haproxy.com> - 2017-03-23 18:40 +0100
    Re: [PATCH 00/13] Add HD44780 Character LCD support Willy TARREAU <wtarreau@haproxy.com> - 2017-03-23 18:40 +0100
      Re: [PATCH 00/13] Add HD44780 Character LCD support Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-23 20:00 +0100
        Re: [PATCH 00/13] Add HD44780 Character LCD support Willy TARREAU <wtarreau@haproxy.com> - 2017-03-23 20:10 +0100
          Re: [PATCH 00/13] Add HD44780 Character LCD support Geert Uytterhoeven <geert@linux-m68k.org> - 2017-03-23 21:30 +0100

#1607738 — Re: [PATCH 00/13] Add HD44780 Character LCD support

FromWilly TARREAU <wtarreau@haproxy.com>
Date2017-03-23 18:40 +0100
SubjectRe: [PATCH 00/13] Add HD44780 Character LCD support
Message-ID<toeSl-5uK-3@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1607743

FromWilly TARREAU <wtarreau@haproxy.com>
Date2017-03-23 18:40 +0100
Message-ID<toeSl-5uK-11@gated-at.bofh.it>
In reply to#1607738
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);
 	}

[toc] | [prev] | [next] | [standalone]

#1607805

FromGeert Uytterhoeven <geert@linux-m68k.org>
Date2017-03-23 20:00 +0100
Message-ID<tog7L-6nW-5@gated-at.bofh.it>
In reply to#1607743
Hi Willy,

On Thu, Mar 23, 2017 at 6:37 PM, Willy TARREAU <wtarreau@haproxy.com> 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 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1607812

FromWilly TARREAU <wtarreau@haproxy.com>
Date2017-03-23 20:10 +0100
Message-ID<toghs-6GO-23@gated-at.bofh.it>
In reply to#1607805
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

[toc] | [prev] | [next] | [standalone]

#1607874

FromGeert Uytterhoeven <geert@linux-m68k.org>
Date2017-03-23 21:30 +0100
Message-ID<tohwR-7sz-9@gated-at.bofh.it>
In reply to#1607812
Hi Willy,

On Thu, Mar 23, 2017 at 8:03 PM, Willy TARREAU <wtarreau@haproxy.com> 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 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web