hpet: Fix missing '=' character in the __setup() code of hpet_mmap_enable

PATCH In review Buland Singh 3 participants 8 messages Dec 20, 2018 – Jan 7, 2019

Subsystems (3)

Stable branch Char and misc drivers Hpet: High Precision Event Timers driver

lore

Share & tools

lore

Buland Singh

author

2018-12-20 12:05 UTC #0 lore raw

PATCH v2 1/1 hpet: Fix missing '=' character in the __setup() code of hpet_mmap_enable

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 <[email protected]>

---
 drivers/char/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/char/hpet.c +1 -1

index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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);
 
 static int hpet_mmap(struct file *file, struct vm_area_struct *vma)
 {

Signature · 1 line

2.19.1

Greg KH

maintainer

2018-12-20 12:29 UTC #1 lore raw

Quoted 38 lines · Buland Singh

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 <[email protected]>
---
 drivers/char/hpet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/char/hpet.c +0 -0
index 4a22b4b41aef..9bffcd37cc7b 100644

drivers/char/hpet.c static __init int hpet_mmap_enable(char *str) · line 380

Quoted 5 lines

drivers/char/hpet.c +1 -1

@@ -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);

This has _never_ worked? Since 3.13? Why not just remove the thing as it is obvious no one actually has ever used it. That would make the code even simpler :)

thanks,

greg k-h

Buland Singh

author

2018-12-20 13:43 UTC #2 lore raw

Quoted 34 lines · Greg KH

Quoted 33 lines · Buland Singh
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 <[email protected]>
---
  drivers/char/hpet.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 10 lines

drivers/char/hpet.c static __init int hpet_mmap_enable(char *str) · line 380

Quoted 10 lines

drivers/char/hpet.c +1 -1

index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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,

Quoted 1 line

This has _never_ worked? Since 3.13?

Yes, that's true :)

Quoted 1 line

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.

Regards,
Buland Singh

[1] https://www.dpdk.org
[2] https://doc.dpdk.org/guides-18.08/linux_gsg/enable_func.html

Greg KH

maintainer

2018-12-20 14:09 UTC #3 lore raw

Quoted 50 lines · Buland Singh

Quoted 34 lines · Greg KH
Quoted 33 lines · Buland Singh
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 <[email protected]>
---
  drivers/char/hpet.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 10 lines
drivers/char/hpet.c static __init int hpet_mmap_enable(char *str) · line 380
Quoted 10 lines
drivers/char/hpet.c +1 -1
index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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,
Quoted 1 line
This has _never_ worked? Since 3.13?

Quoted 7 lines

Yes, that's true :)
Quoted 1 line
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:

Quoted 1 line

[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

Buland Singh

author

2018-12-21 12:18 UTC #4 lore raw

Quoted 64 lines · Greg KH

Quoted 50 lines · Buland Singh
Quoted 34 lines · Greg KH
Quoted 33 lines · Buland Singh
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 <[email protected]>
---
   drivers/char/hpet.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 10 lines
drivers/char/hpet.c static __init int hpet_mmap_enable(char *str) · line 380
Quoted 10 lines
drivers/char/hpet.c +1 -1
index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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,
Quoted 1 line
This has _never_ worked? Since 3.13?
Quoted 7 lines
Yes, that's true :)
Quoted 1 line
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:

Quoted 9 lines

Quoted 1 line
[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.

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.

Regards,
Buland Singh

Greg KH

maintainer

2018-12-21 12:46 UTC #5 lore raw

Quoted 72 lines · Buland Singh

Quoted 71 lines · Greg KH
Quoted 50 lines · Buland Singh
Quoted 34 lines · Greg KH
Quoted 33 lines · Buland Singh
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 <[email protected]>
---
   drivers/char/hpet.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 10 lines
drivers/char/hpet.c static __init int hpet_mmap_enable(char *str) · line 380
Quoted 10 lines
drivers/char/hpet.c +1 -1
index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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,
Quoted 1 line
This has _never_ worked? Since 3.13?
Quoted 7 lines
Yes, that's true :)
Quoted 1 line
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:
Quoted 1 line
[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,

Quoted 7 lines

Quoted 2 lines
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.

Quoted 5 lines

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

Buland Singh

author

2019-01-06 14:09 UTC #6 lore raw

Quoted 91 lines · Greg KH

Quoted 72 lines · Buland Singh
Quoted 71 lines · Greg KH
Quoted 50 lines · Buland Singh
Quoted 34 lines · Greg KH
Quoted 33 lines · Buland Singh
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 <[email protected]>
---
    drivers/char/hpet.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 10 lines
drivers/char/hpet.c static __init int hpet_mmap_enable(char *str) · line 380
Quoted 10 lines
drivers/char/hpet.c +1 -1
index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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,
Quoted 1 line
This has _never_ worked? Since 3.13?
Quoted 7 lines
Yes, that's true :)
Quoted 1 line
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:
Quoted 1 line
[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,
Quoted 7 lines
Quoted 2 lines
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.
Quoted 5 lines
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.

Quoted 8 lines

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

Prarit Bhargava

2019-01-07 12:40 UTC #7 lore raw

Quoted 97 lines · Buland Singh

Quoted 96 lines · Greg KH
Quoted 77 lines · Buland Singh
Quoted 76 lines · Greg KH
Quoted 54 lines · Buland Singh
Quoted 35 lines · Greg KH
Quoted 34 lines · Buland Singh
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 <[email protected]>
---
    drivers/char/hpet.c | 2 +-
    1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 11 lines
drivers/char/hpet.c
Quoted 11 lines
drivers/char/hpet.c +0 -0
index 4a22b4b41aef..9bffcd37cc7b 100644
@@ -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;
    }
drivers/char/hpet.c +1 -1
-__setup("hpet_mmap", hpet_mmap_enable);
+__setup("hpet_mmap=", hpet_mmap_enable);
Hello Greag,
Quoted 1 line
This has _never_ worked? Since 3.13?
Yes, that's true :)
Quoted 8 lines
Quoted 2 lines
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:
Quoted 1 line
[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,
Quoted 7 lines
Quoted 2 lines
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.
Quoted 5 lines
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.

Quoted 4 lines

Quoted 4 lines
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?

The feature came about when two customers asked for it. The customers are willing to live with the increased security risk with fast HPET MMAP access.

The only place I've seen a use request for this is in RHEL, and the bug was likely uncovered during RHEL8 beta testing which explains why no one has used the upstream code. ie) RHEL7 does contain an "=" sign, upstream/RHEL8 do not.

Quoted 15 lines

Quoted 4 lines
thanks,
greg k-h
Hello,
Adding Prarit to cc list for his opinion.
Regards,
Signature · 1 line
Buland Singh