[PATCH 1/1] char: hpet: Remove unused local variable 'm' in hpet_interrupt()

[PATCH 1/1] char: hpet: Remove unused local variable 'm' in hpet_interrupt()

[Zhen Lei]

Commit 273ef9509b79 ("drivers/char/hpet.c: fix periodic-emulation for
delayed interrupt") removed the reference to local variable 'm', but
forgot to remove the definition and assignment of it. Due to
read_counter() indirectly calls "read barrier", the performance is
slightly degraded.

Since the following comments give some description based on 'm', so move
the assignment of 'm' into it.

Fixes: 273ef9509b79 ("drivers/char/hpet.c: fix periodic-emulation for delayed interrupt")
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/char/hpet.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
index ed3b7dab678dbd1..46950a0cda181a1 100644
--- a/drivers/char/hpet.c
+++ b/drivers/char/hpet.c
@@ -156,14 +156,16 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
 	 * This has the effect of treating non-periodic like periodic.
 	 */
 	if ((devp->hd_flags & (HPET_IE | HPET_PERIODIC)) == HPET_IE) {
-		unsigned long m, t, mc, base, k;
+		unsigned long t, mc, base, k;
 		struct hpet __iomem *hpet = devp->hd_hpet;
 		struct hpets *hpetp = devp->hd_hpets;
 
 		t = devp->hd_ireqfreq;
-		m = read_counter(&devp->hd_timer->hpet_compare);
 		mc = read_counter(&hpet->hpet_mc);
-		/* The time for the next interrupt would logically be t + m,
+		/*
+		 * m = read_counter(&devp->hd_timer->hpet_compare);
+		 *
+		 * The time for the next interrupt would logically be t + m,
 		 * however, if we are very unlucky and the interrupt is delayed
 		 * for longer than t then we will completely miss the next
 		 * interrupt if we set t + m and an application will hang.
-- 
2.26.0.106.g9fadedd

Re: [PATCH 1/1] char: hpet: Remove unused local variable 'm' in hpet_interrupt()

[Greg Kroah-Hartman]

On Thu, Apr 15, 2021 at 10:24:04PM +0800, Zhen Lei wrote:
> Commit 273ef9509b79 ("drivers/char/hpet.c: fix periodic-emulation for
> delayed interrupt") removed the reference to local variable 'm', but
> forgot to remove the definition and assignment of it. Due to
> read_counter() indirectly calls "read barrier", the performance is
> slightly degraded.
> 
> Since the following comments give some description based on 'm', so move
> the assignment of 'm' into it.
> 
> Fixes: 273ef9509b79 ("drivers/char/hpet.c: fix periodic-emulation for delayed interrupt")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  drivers/char/hpet.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
> index ed3b7dab678dbd1..46950a0cda181a1 100644
> --- a/drivers/char/hpet.c
> +++ b/drivers/char/hpet.c
> @@ -156,14 +156,16 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
>  	 * This has the effect of treating non-periodic like periodic.
>  	 */
>  	if ((devp->hd_flags & (HPET_IE | HPET_PERIODIC)) == HPET_IE) {
> -		unsigned long m, t, mc, base, k;
> +		unsigned long t, mc, base, k;
>  		struct hpet __iomem *hpet = devp->hd_hpet;
>  		struct hpets *hpetp = devp->hd_hpets;
>  
>  		t = devp->hd_ireqfreq;
> -		m = read_counter(&devp->hd_timer->hpet_compare);
>  		mc = read_counter(&hpet->hpet_mc);
> -		/* The time for the next interrupt would logically be t + m,
> +		/*
> +		 * m = read_counter(&devp->hd_timer->hpet_compare);

Why did you comment this out?

And are you sure that yuou are not required to actually read that
counter, even if you do not do anything with the value?  Lots of
hardware works in odd ways...

Have you tested and verified that this still works properly?

thanks,

greg k-h

Re: [PATCH 1/1] char: hpet: Remove unused local variable 'm' in hpet_interrupt()

[Leizhen (ThunderTown)]

On 2021/4/15 22:53, Greg Kroah-Hartman wrote:
> On Thu, Apr 15, 2021 at 10:24:04PM +0800, Zhen Lei wrote:
>> Commit 273ef9509b79 ("drivers/char/hpet.c: fix periodic-emulation for
>> delayed interrupt") removed the reference to local variable 'm', but
>> forgot to remove the definition and assignment of it. Due to
>> read_counter() indirectly calls "read barrier", the performance is
>> slightly degraded.
>>
>> Since the following comments give some description based on 'm', so move
>> the assignment of 'm' into it.
>>
>> Fixes: 273ef9509b79 ("drivers/char/hpet.c: fix periodic-emulation for delayed interrupt")
>> Reported-by: Hulk Robot <hulkci@huawei.com>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  drivers/char/hpet.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/char/hpet.c b/drivers/char/hpet.c
>> index ed3b7dab678dbd1..46950a0cda181a1 100644
>> --- a/drivers/char/hpet.c
>> +++ b/drivers/char/hpet.c
>> @@ -156,14 +156,16 @@ static irqreturn_t hpet_interrupt(int irq, void *data)
>>  	 * This has the effect of treating non-periodic like periodic.
>>  	 */
>>  	if ((devp->hd_flags & (HPET_IE | HPET_PERIODIC)) == HPET_IE) {
>> -		unsigned long m, t, mc, base, k;
>> +		unsigned long t, mc, base, k;
>>  		struct hpet __iomem *hpet = devp->hd_hpet;
>>  		struct hpets *hpetp = devp->hd_hpets;
>>  
>>  		t = devp->hd_ireqfreq;
>> -		m = read_counter(&devp->hd_timer->hpet_compare);
>>  		mc = read_counter(&hpet->hpet_mc);
>> -		/* The time for the next interrupt would logically be t + m,
>> +		/*
>> +		 * m = read_counter(&devp->hd_timer->hpet_compare);
> 
> Why did you comment this out?
> 
> And are you sure that yuou are not required to actually read that
> counter, even if you do not do anything with the value?  Lots of
> hardware works in odd ways...
> 
> Have you tested and verified that this still works properly?

Sorry, I didn't actually test it. I didn't see any dependency on
this read operation for other members' reads and writes. If this
read operation is potentially required, hopefully there is an
explanatory note next to it.

> 
> thanks,
> 
> greg k-h
> 
> .
>