linux-kernel - Re: [PATCH 1/3] auxdisplay: deconfuse configuration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 06 Mar 2019 15:08:31 +0000
From:   Måns Rullgård <mans@...sr.com>
To:     Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Ksenija Stanojevic <ksenija.stanojevic@...il.com>,
        Willy Tarreau <w@....eu>,
        Geert Uytterhoeven <geert.uytterhoeven@...il.com>
Subject: Re: [PATCH 1/3] auxdisplay: deconfuse configuration

Miguel Ojeda <miguel.ojeda.sandonis@...il.com> writes:

> On Fri, Mar 1, 2019 at 7:48 PM Mans Rullgard <mans@...sr.com> wrote:
>>
>> The auxdisplay Kconfig is confusing.  It creates two separate menus
>> even though the settings are closely related.  Moreover, the options
>> for setting the boot message depend on CONFIG_PARPORT even though they
>> are used by drivers that do not.
>>
>> Clear up the confustion by moving the "Parallel port LCD/Keypad" menu
>
> "confustion" -> "confusion"

Darn, must have been confused while typing.

>> under auxdisplay where it logically belongs.  Change the boot message
>> options to depend only on CONFIG_CHARLCD, making them accessible also
>> when only the HD44780 is selected.
>>
>> Since the "Parallel port LCD/Keypad" driver now has a new dependency
>> on CONFIG_AUXDISPLAY, rename its Kconfig symbol and keep the old one
>> such that make oldconfig will not disable the driver.
>>
>> Signed-off-by: Mans Rullgard <mans@...sr.com>
>> ---
>>  drivers/auxdisplay/Kconfig  | 17 ++++++++++++-----
>>  drivers/auxdisplay/Makefile |  2 +-
>>  2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
>> index 57410f9c5d44..7d3fe27d6868 100644
>> --- a/drivers/auxdisplay/Kconfig
>> +++ b/drivers/auxdisplay/Kconfig
>> @@ -164,9 +164,7 @@ config ARM_CHARLCD
>>           line and the Linux version on the second line, but that's
>>           still useful.
>>
>> -endif # AUXDISPLAY
>> -
>> -menuconfig PANEL
>> +menuconfig PARPORT_PANEL
>
> Do we want the PARPORT_ prefix here but not on the suboptions?

I was trying to bring some sanity to it without changing more than
necessary.

> Also, having PANEL_PARPORT and PARPORT_PANEL seems confusing...

It is...

>>         tristate "Parallel port LCD/Keypad Panel support"
>>         depends on PARPORT
>>         select CHARLCD
>> @@ -178,7 +176,7 @@ menuconfig PANEL
>>           compiled as a module, or linked into the kernel and started at boot.
>>           If you don't understand what all this is about, say N.
>>
>> -if PANEL
>> +if PARPORT_PANEL
>>
>>  config PANEL_PARPORT
>>         int "Default parallel port number (0=LPT1)"
>> @@ -419,8 +417,11 @@ config PANEL_LCD_PIN_BL
>>
>>           Default for the 'BL' pin in custom profile is '0' (uncontrolled).
>>
>> +endif # PARPORT_PANEL
>> +
>>  config PANEL_CHANGE_MESSAGE
>>         bool "Change LCD initialization message ?"
>> +       depends on CHARLCD
>>         default "n"
>>         ---help---
>>           This allows you to replace the boot message indicating the kernel version
>> @@ -444,7 +445,13 @@ config PANEL_BOOT_MESSAGE
>>           An empty message will only clear the display at driver init time. Any other
>>           printf()-formatted message is valid with newline and escape codes.
>>
>> -endif # PANEL
>> +endif # AUXDISPLAY
>> +
>> +config PANEL
>> +       tristate "Parallel port LCD/Keypad Panel support (OLD OPTION)"
>
> Hm... what do you mean by "OLD OPTION"? Should we keep it? (I don't
> see any other place using this marking).

The option is there so 'make oldconfig' on existing configurations
doesn't silently drop that driver since it now depends on AUXDISPLAY.
There have been similar cases when shuffling options.  Maybe they used a
different tag.  We could of course let the three people using that
driver deal with it themselves.

>> +       depends on PARPORT
>> +       select AUXDISPLAY
>> +       select PARPORT_PANEL
>
> I agree the menu was a bit convoluted and we didn't get to clean it.
>
> Since you are touching the panel.c options, CC'ing the maintainers
> (please do run get_maintainer.pl in that case!)

I always run get_maintainer.pl on patches.  Sometimes it isn't clever
enough to figure out all the people who might be interested.

-- 
Måns Rullgård