diff mbox series

clocksource/drivers/arm_arch_timer: Fix event stream param in Kconfig

Message ID 20221024195118.871907-1-code@tyhicks.com
State New
Headers
Series clocksource/drivers/arm_arch_timer: Fix event stream param in Kconfig |

Commit Message

Tyler Hicks Oct. 24, 2022, 7:51 p.m. UTC 
  From: "Tyler Hicks (Microsoft)" <code@tyhicks.com>

Fix the event stream timer command line parameter name that's documented
in the Kconfig description for CONFIG_ARM_ARCH_TIMER_EVTSTREAM. It
didn't match the command line parameter name that's actually honored in
the source code.

Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
Fixes: 46fd5c6b3059 ("clocksource/drivers/arm_arch_timer: Control the evtstrm via the cmdline")
Cc: stable@vger.kernel.org
Signed-off-by: Tyler Hicks (Microsoft) <code@tyhicks.com>
---
 drivers/clocksource/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marc Zyngier Oct. 25, 2022, 10:24 a.m. UTC | #1 
Hi Tyler,

On Mon, 24 Oct 2022 20:51:18 +0100,
Tyler Hicks <code@tyhicks.com> wrote:
> 
> From: "Tyler Hicks (Microsoft)" <code@tyhicks.com>
> 
> Fix the event stream timer command line parameter name that's documented
> in the Kconfig description for CONFIG_ARM_ARCH_TIMER_EVTSTREAM. It
> didn't match the command line parameter name that's actually honored in
> the source code.
> 
> Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
> Fixes: 46fd5c6b3059 ("clocksource/drivers/arm_arch_timer: Control the evtstrm via the cmdline")
> Cc: stable@vger.kernel.org

No, this really doesn't deserve a Cc: stable. This bit may be wrong,
but we have the correct information in the kernel-parameters.txt file,
which is authoritative AFAIC.

> Signed-off-by: Tyler Hicks (Microsoft) <code@tyhicks.com>
> ---
>  drivers/clocksource/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 4469e7f555e9..dcc274460973 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -315,7 +315,7 @@ config ARM_ARCH_TIMER_EVTSTREAM
>  	  executing the wfe instruction at a frequency represented as a
>  	  power-of-2 divisor of the clock rate. The behaviour can also be
>  	  overridden on the command line using the
> -	  clocksource.arm_arch_timer.evtstream parameter.
> +	  clocksource.arm_arch_timer.evtstrm parameter.
>  	  The main use of the event stream is wfe-based timeouts of userspace
>  	  locking implementations. It might also be useful for imposing timeout
>  	  on wfe to safeguard against any programming errors in case an expected

With this tag dropped,

Acked-by: Marc Zyngier <maz@kernel.org>

	M.
Tyler Hicks Oct. 25, 2022, 2:18 p.m. UTC | #2 
On 2022-10-25 11:24:55, Marc Zyngier wrote:
> Hi Tyler,
> 
> On Mon, 24 Oct 2022 20:51:18 +0100,
> Tyler Hicks <code@tyhicks.com> wrote:
> > 
> > From: "Tyler Hicks (Microsoft)" <code@tyhicks.com>
> > 
> > Fix the event stream timer command line parameter name that's documented
> > in the Kconfig description for CONFIG_ARM_ARCH_TIMER_EVTSTREAM. It
> > didn't match the command line parameter name that's actually honored in
> > the source code.
> > 
> > Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
> > Fixes: 46fd5c6b3059 ("clocksource/drivers/arm_arch_timer: Control the evtstrm via the cmdline")
> > Cc: stable@vger.kernel.org
> 
> No, this really doesn't deserve a Cc: stable. This bit may be wrong,
> but we have the correct information in the kernel-parameters.txt file,
> which is authoritative AFAIC.

Thanks for the review!

I added the stable tag because it caused an actual waste of time in an
investigation. Upon discovering this option, I didn't notice the subtle
difference in cmdline parameter name from the Kconfig, to the
kernel-parameters.txt file and the code. I ran long-running stress tests
with the incorrect parameter name copied and pasted from the Kconfig.
This error resulted in a bad data point in the investigation and it took
a second set of eyes (Vijay's) to uncover my mistake days later when we
were trying to reconcile the bad data point with other good data points.

It is no big deal if it is merged with the tag dropped but I wanted to
provide my reasoning of why I personally felt this was a more than just
a comment typo fix or something along those lines. Thanks, again!

Tyler

> 
> > Signed-off-by: Tyler Hicks (Microsoft) <code@tyhicks.com>
> > ---
> >  drivers/clocksource/Kconfig | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index 4469e7f555e9..dcc274460973 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -315,7 +315,7 @@ config ARM_ARCH_TIMER_EVTSTREAM
> >  	  executing the wfe instruction at a frequency represented as a
> >  	  power-of-2 divisor of the clock rate. The behaviour can also be
> >  	  overridden on the command line using the
> > -	  clocksource.arm_arch_timer.evtstream parameter.
> > +	  clocksource.arm_arch_timer.evtstrm parameter.
> >  	  The main use of the event stream is wfe-based timeouts of userspace
> >  	  locking implementations. It might also be useful for imposing timeout
> >  	  on wfe to safeguard against any programming errors in case an expected
> 
> With this tag dropped,
> 
> Acked-by: Marc Zyngier <maz@kernel.org>
> 
> 	M.
> 
> -- 
> Without deviation from the norm, progress is not possible.
Marc Zyngier Oct. 25, 2022, 3:07 p.m. UTC | #3 
Hi Tyler,

On Tue, 25 Oct 2022 15:18:54 +0100,
"Tyler Hicks (Microsoft)" <code@tyhicks.com> wrote:
> 
> On 2022-10-25 11:24:55, Marc Zyngier wrote:
> > Hi Tyler,
> > 
> > On Mon, 24 Oct 2022 20:51:18 +0100,
> > Tyler Hicks <code@tyhicks.com> wrote:
> > > 
> > > From: "Tyler Hicks (Microsoft)" <code@tyhicks.com>
> > > 
> > > Fix the event stream timer command line parameter name that's documented
> > > in the Kconfig description for CONFIG_ARM_ARCH_TIMER_EVTSTREAM. It
> > > didn't match the command line parameter name that's actually honored in
> > > the source code.
> > > 
> > > Reported-by: Vijay Balakrishna <vijayb@linux.microsoft.com>
> > > Fixes: 46fd5c6b3059 ("clocksource/drivers/arm_arch_timer: Control the evtstrm via the cmdline")
> > > Cc: stable@vger.kernel.org
> > 
> > No, this really doesn't deserve a Cc: stable. This bit may be wrong,
> > but we have the correct information in the kernel-parameters.txt file,
> > which is authoritative AFAIC.
> 
> Thanks for the review!
> 
> I added the stable tag because it caused an actual waste of time in an
> investigation. Upon discovering this option, I didn't notice the subtle
> difference in cmdline parameter name from the Kconfig, to the
> kernel-parameters.txt file and the code. I ran long-running stress tests
> with the incorrect parameter name copied and pasted from the Kconfig.
> This error resulted in a bad data point in the investigation and it took
> a second set of eyes (Vijay's) to uncover my mistake days later when we
> were trying to reconcile the bad data point with other good data points.

I appreciate this has caused a certain waste of time, but the rules
for stable inclusion are pretty clear: it has to be a runtime bug.
Documentation bugs, as irritating as they are, do not fall into this
category (it really *is* a typo).

Thanks,

	M.
diff mbox series

Patch

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index 4469e7f555e9..dcc274460973 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -315,7 +315,7 @@  config ARM_ARCH_TIMER_EVTSTREAM
 	  executing the wfe instruction at a frequency represented as a
 	  power-of-2 divisor of the clock rate. The behaviour can also be
 	  overridden on the command line using the
-	  clocksource.arm_arch_timer.evtstream parameter.
+	  clocksource.arm_arch_timer.evtstrm parameter.
 	  The main use of the event stream is wfe-based timeouts of userspace
 	  locking implementations. It might also be useful for imposing timeout
 	  on wfe to safeguard against any programming errors in case an expected