Re: [PATCH 09/13] auxdisplay: charlcd: Extract character LCD core from misc/panel

[Geert Uytterhoeven]

On Mon, Feb 6, 2017 at 3:38 PM, Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> Extract the character LCD core from the Parallel port LCD/Keypad Panel
> driver in the misc subsystem, and convert it into a subdriver in the
> auxdisplay subsystem.  This allows the character LCD core to be used by
> other drivers later.
>
> Compilation is controlled by its own Kconfig symbol CHARLCD, which is to
> be selected by its users, but can be enabled manually for
> compile-testing.
>
> All functions changed their prefix from "lcd_" to "charlcd_", and gained
> a "struct charlcd *" parameter to operate on a specific instance.
> While the driver API thus is ready to support multiple instances, the
> current limitation of a single display (/dev/lcd has a single misc minor
> assigned) is retained.
>
> No functional changes intended.
>
> Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>

> --- /dev/null
> +++ b/drivers/auxdisplay/charlcd.c

> +/* initialize the LCD driver */
> +static int charlcd_init(struct charlcd *lcd)
> +{
> +       struct charlcd_priv *priv = to_priv(lcd);
> +       int ret;
> +
> +       /*
> +        * before this line, we must NOT send anything to the display.
> +        * Since charlcd_init_display() needs to write data, we have to
> +        * enable mark the LCD initialized just before.
> +        */
> +       ret = charlcd_init_display(lcd);

0day reported:

| INFO: trying to register non-static key.
| the code is fine but needs lockdep annotation.
| turning off the locking correctness validator.

However, the code is not fine.
charlcd_init_display() calls charlcd_backlight(), which acquires the
mutex that is only initialized below.

> +       if (ret)
> +               return ret;
> +
> +       if (lcd->ops->backlight) {
> +               mutex_init(&priv->bl_tempo_lock);
> +               INIT_DELAYED_WORK(&priv->bl_work, charlcd_bl_off);
> +       }

Moving the block above before the call to charlcd_init_display() fixes this.
Will do so in v2.

Note that "misc: panel: Abstract temporary backlight handling" doesn't
suffer from this bug.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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