linux-kernel - Re: [PATCH v3] misc: bcm-vk: only support ttyVK if CONFIG

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]

Message-ID: <30098c21-f0aa-55f3-8e1f-c7bb8ae8be0e@broadcom.com>
Date:   Mon, 1 Feb 2021 10:13:55 -0800
From:   Scott Branden <scott.branden@...adcom.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Arnd Bergmann <arnd@...db.de>, Kees Cook <keescook@...omium.org>,
        linux-kernel@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Olof Johansson <olof@...om.net>,
        Desmond Yan <desmond.yan@...adcom.com>
Subject: Re: [PATCH v3] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

Hi Greg,,

I need a few clarifications before sending (hopefully) final revisions to the patch.

On 2021-01-31 11:45 p.m., Greg Kroah-Hartman wrote:
> On Sun, Jan 31, 2021 at 03:30:49PM -0800, Scott Branden wrote:
>> Correct compile issue if CONFIG_TTY is not set by
>> only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
>>
>> Reported-by: Randy Dunlap <rdunlap@...radead.org>
>> Signed-off-by: Scott Branden <scott.branden@...adcom.com>
>>
>> ---
>> Changes since v2:
>> - add CONFIG_BCM_VK_TTY
>> - add function and stub for bcm_vk_tty_set_irq_enabled
>> Changes since v1:
>> - add function stubs rather than compiling out code
>> ---
>>  drivers/misc/bcm-vk/Kconfig      | 16 ++++++++++++
>>  drivers/misc/bcm-vk/Makefile     |  4 +--
>>  drivers/misc/bcm-vk/bcm_vk.h     | 42 +++++++++++++++++++++++++++++---
>>  drivers/misc/bcm-vk/bcm_vk_dev.c |  5 ++--
>>  drivers/misc/bcm-vk/bcm_vk_tty.c |  6 +++++
>>  5 files changed, 65 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
>> index 052f6f28b540..16ce98c964b8 100644
>> --- a/drivers/misc/bcm-vk/Kconfig
>> +++ b/drivers/misc/bcm-vk/Kconfig
>> @@ -15,3 +15,19 @@ config BCM_VK
>>  	  accelerators via /dev/bcm-vk.N devices.
>>  
>>  	  If unsure, say N.
>> +
>> +if BCM_VK
> No need for this, just put it on the depends line, right?
If you prefer I can but it on the depends on line.
But, I actually prefer the if syntax in this case as it more clearly shows
BCM_VK_TTY is a suboption of BCM_VK.

Please let me know which method is "right"?
>
>> +
>> +config BCM_VK_TTY
>> +	bool "Enable ttyVK"
> Better config help text to explain what this is?
I'll change it to the following?

"Enable tty's on VK devices"
>
>> +	depends on TTY
>> +	default y
> Default y is only there if your system can not boot without it, please
> remove it.
I can remove if really needed but I'd like to learn more about such convention.
Is there a document I can learn from describing such?

We actually want a full featured driver by default.  Otherwise we'll end up asking people to enable this
feature and recompile the driver to get missing features such as this.
>
>> +	help
>> +	  Select this option to enable ttyVK support to allow console
>> +	  access to VK cards from host.
> Again, more help text, what is a "VK"?
VK is already described in BCM_VK.  Why would I need to add the same information again to a suboption?
Perhaps you would like "config BCM_VK" changed to "menuconfig BCM_VK"
>
> thanks,
>
> greg k-h
Thanks,
 Scott