Re: [PATCH v2] hpet: Fix missing '=' character in the __setup() code of hpet_mmap_enable

[Buland Singh <bsingh@redhat.com>]

On 12/21/18 6:16 PM, Greg KH wrote:
> On Fri, Dec 21, 2018 at 05:48:38PM +0530, Buland Singh wrote:
>> On 12/20/18 7:39 PM, Greg KH wrote:
>>> On Thu, Dec 20, 2018 at 07:12:55PM +0530, Buland Singh wrote:
>>>> On 12/20/18 5:59 PM, Greg KH wrote:
>>>>> On Thu, Dec 20, 2018 at 05:35:24PM +0530, Buland Singh wrote:
>>>>>> Commit '3d035f580699 ("drivers/char/hpet.c: allow user controlled mmap for
>>>>>> user processes")' introduced a new kernel command line parameter hpet_mmap,
>>>>>> that is required to expose the memory map of the HPET registers to
>>>>>> user-space. Unfortunately the kernel command line parameter 'hpet_mmap' is
>>>>>> broken and never takes effect due to missing '=' character in the __setup()
>>>>>> code of hpet_mmap_enable.
>>>>>>
>>>>>> Before this patch:
>>>>>>
>>>>>> dmesg output with the kernel command line parameter hpet_mmap=1
>>>>>>
>>>>>> [    0.204152] HPET mmap disabled
>>>>>>
>>>>>> dmesg output with the kernel command line parameter hpet_mmap=0
>>>>>>
>>>>>> [    0.204192] HPET mmap disabled
>>>>>>
>>>>>> After this patch:
>>>>>>
>>>>>> dmesg output with the kernel command line parameter hpet_mmap=1
>>>>>>
>>>>>> [    0.203945] HPET mmap enabled
>>>>>>
>>>>>> dmesg output with the kernel command line parameter hpet_mmap=0
>>>>>>
>>>>>> [    0.204652] HPET mmap disabled
>>>>>>
>>>>>> Fixes: 3d035f580699 ("drivers/char/hpet.c: allow user controlled mmap for user processes")
>>>>>> Signed-off-by: Buland Singh <bsingh@redhat.com>
>>>>>> ---
>>>>>>     drivers/char/hpet.c | 2 +-
>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
>>>>>> index 4a22b4b41aef..9bffcd37cc7b 100644
>>>>>> --- a/drivers/char/hpet.c
>>>>>> +++ b/drivers/char/hpet.c
>>>>>> @@ -377,7 +377,7 @@ static __init int hpet_mmap_enable(char *str)
>>>>>>     	pr_info("HPET mmap %s\n", hpet_mmap_enabled ? "enabled" : "disabled");
>>>>>>     	return 1;
>>>>>>     }
>>>>>> -__setup("hpet_mmap", hpet_mmap_enable);
>>>>>> +__setup("hpet_mmap=", hpet_mmap_enable);
>>>>
>>>> Hello Greag,
>>>>
>>>>> This has _never_ worked?  Since 3.13?
>>>>
>>>> Yes, that's true :)
>>>>
>>>>> Why not just remove the thing as it is obvious no one actually has ever used it.  > That would make the code even simpler :)
>>>>
>>>> Data Plane Development Kit (DPDK)[1] provides API that requires the CONFIG_HPET_MMAP
>>>> kernel configuration option to be enabled[2]. Some end users might want to use the
>>>> HPET MMAP functionality within the application.
>>>
>>> But, obviously, they really don't need to do that from the kernel
>>> command line as no one has ever noticed this didn't work :)
>>>
>>> Also, that page:
>>>
>>>> [2] https://doc.dpdk.org/guides-18.08/linux_gsg/enable_func.html
>>>
>>> Does not say to use this command line option either.  So if no one has
>>> ever used it, please, let us just delete it.
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> Hello Greg,
>> It is better to allow a user to 'enable/disable' the HPET mmap from the
>> kernel command line as per the requirement rather than recompiling the
>> kernel to 'enable/disable' this functionality.
> 
> It might be "nice" but given that no one has noticed that this has never
> worked, for all of the years that this has been present, that means to
> me that no one has ever tried it because they really do not "need" it.
> 
>> Also as per the description in the initial patch (commit 3d035f58), the
>> 'CONFIG_HPET_MMAP' Kconfig option has a security risk involved. Hence,
>> keeping the CONFIG_HPET_MMAP_DEFAULT (disabled) and allowing a user to
>> alter the default behavior using the kernel command line parameter
>> hpet_mmap is a better solution.
> 
> Again, because no one has ever noticed that this was not working, why
> not just rip it out until someone speaks up as to why they have to have
> this feature and the other alternatives do not work for them?
> 
> thanks,
> 
> greg k-h
> 

Hello,

Adding Prarit to cc list for his opinion.

Regards,
--
Buland Singh