Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Damien Zammit]

>From: Michael Tokarev <mjt@tls.msk.ru>
>26.02.2023 04:58, Damien Zammit wrote:
>> Currently, the one-shot (mode 1) PIT expires far too quickly,
>> due to the output being set under the wrong logic.
>> This change fixes the one-shot PIT mode to behave similarly to mode 0.
>> 
>> TESTED: using the one-shot PIT mode to calibrate a local apic timer.
>
>Has this been forgotten, or is it not needed anymore?

This is still required but nobody uses the
PIT one-shot mode (probably because it *is* currently broken).

Can it be merged?

Thanks,
Damien

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Michael S. Tsirkin]

On Fri, Oct 06, 2023 at 02:36:52AM +0000, Damien Zammit wrote:
> >From: Michael Tokarev <mjt@tls.msk.ru>
> >26.02.2023 04:58, Damien Zammit wrote:
> >> Currently, the one-shot (mode 1) PIT expires far too quickly,
> >> due to the output being set under the wrong logic.
> >> This change fixes the one-shot PIT mode to behave similarly to mode 0.
> >> 
> >> TESTED: using the one-shot PIT mode to calibrate a local apic timer.
> >
> >Has this been forgotten, or is it not needed anymore?
> 
> This is still required but nobody uses the
> PIT one-shot mode (probably because it *is* currently broken).
> 
> Can it be merged?
> 
> Thanks,
> Damien

OK, I tagged it. thanks!

[PATCH qemu] timer/i8254: Fix one shot PIT mode

[Damien Zammit]

Currently, the one-shot (mode 1) PIT expires far too quickly,
due to the output being set under the wrong logic.
This change fixes the one-shot PIT mode to behave similarly to mode 0.

TESTED: using the one-shot PIT mode to calibrate a local apic timer.

Signed-off-by: Damien Zammit <damien@zamaudio.com>

---
 hw/timer/i8254_common.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
index 050875b497..9164576ca9 100644
--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -52,10 +52,8 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
     switch (s->mode) {
     default:
     case 0:
-        out = (d >= s->count);
-        break;
     case 1:
-        out = (d < s->count);
+        out = (d >= s->count);
         break;
     case 2:
         if ((d % s->count) == 0 && d != 0) {
--
2.39.0

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Michael S. Tsirkin]

On Sun, Feb 26, 2023 at 01:58:10AM +0000, Damien Zammit wrote:
> Currently, the one-shot (mode 1) PIT expires far too quickly,
> due to the output being set under the wrong logic.
> This change fixes the one-shot PIT mode to behave similarly to mode 0.
> 
> TESTED: using the one-shot PIT mode to calibrate a local apic timer.
> 
> Signed-off-by: Damien Zammit <damien@zamaudio.com>
> 
> ---
>  hw/timer/i8254_common.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
> index 050875b497..9164576ca9 100644
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -52,10 +52,8 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
>      switch (s->mode) {
>      default:
>      case 0:
> -        out = (d >= s->count);
> -        break;


I think you need something like
	/* FALLTHRU */
here otherwise some gcc versions will warn.

>      case 1:
> -        out = (d < s->count);
> +        out = (d >= s->count);
>          break;
>      case 2:
>          if ((d % s->count) == 0 && d != 0) {
> --
> 2.39.0
> 

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Damien Zammit]

Hi Michael,

Thanks for reviewing this on a weekend!

On 26/2/23 19:51, Michael S. Tsirkin wrote:
> On Sun, Feb 26, 2023 at 01:58:10AM +0000, Damien Zammit wrote:
>>       case 0:
>> -        out = (d >= s->count);
>> -        break;
>
>
> I think you need something like
> 	/* FALLTHRU */
> here otherwise some gcc versions will warn.
>
>>       case 1:
>> -        out = (d < s->count);
>> +        out = (d >= s->count);

It seems that there are quite a number of these consecutive fallthrough cases
without /* FALLTHRU */ in i8254_common.c

Can these be fixed in a separate patch?

Damien

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Max Filippov]

On Sun, Feb 26, 2023 at 1:18 AM Damien Zammit <damien@zamaudio.com> wrote:
>
> Hi Michael,
>
> Thanks for reviewing this on a weekend!
>
> On 26/2/23 19:51, Michael S. Tsirkin wrote:
> > On Sun, Feb 26, 2023 at 01:58:10AM +0000, Damien Zammit wrote:
> >>       case 0:
> >> -        out = (d >= s->count);
> >> -        break;
> >
> >
> > I think you need something like
> >       /* FALLTHRU */
> > here otherwise some gcc versions will warn.
> >
> >>       case 1:
> >> -        out = (d < s->count);
> >> +        out = (d >= s->count);
>
> It seems that there are quite a number of these consecutive fallthrough cases
> without /* FALLTHRU */ in i8254_common.c
>
> Can these be fixed in a separate patch?

I believe that the comment is only needed when there's code
between the labels and is not needed between the labels that
follow each other.

-- 
Thanks.
-- Max

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Sun, 26 Feb 2023, Max Filippov wrote:
> On Sun, Feb 26, 2023 at 1:18 AM Damien Zammit <damien@zamaudio.com> wrote:
>>
>> Hi Michael,
>>
>> Thanks for reviewing this on a weekend!
>>
>> On 26/2/23 19:51, Michael S. Tsirkin wrote:
>>> On Sun, Feb 26, 2023 at 01:58:10AM +0000, Damien Zammit wrote:
>>>>       case 0:
>>>> -        out = (d >= s->count);
>>>> -        break;
>>>
>>>
>>> I think you need something like
>>>       /* FALLTHRU */
>>> here otherwise some gcc versions will warn.
>>>
>>>>       case 1:
>>>> -        out = (d < s->count);
>>>> +        out = (d >= s->count);
>>
>> It seems that there are quite a number of these consecutive fallthrough cases
>> without /* FALLTHRU */ in i8254_common.c
>>
>> Can these be fixed in a separate patch?
>
> I believe that the comment is only needed when there's code
> between the labels and is not needed between the labels that
> follow each other.

I think so too, I have some of these consecutive case labels in my code 
and never had a problem with that. Only when you have a statement between 
labels without break is when a comment is needed.

Regards,
BALATON Zoltan

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Michael S. Tsirkin]

On Sun, Feb 26, 2023 at 01:11:19PM +0100, BALATON Zoltan wrote:
> On Sun, 26 Feb 2023, Max Filippov wrote:
> > On Sun, Feb 26, 2023 at 1:18 AM Damien Zammit <damien@zamaudio.com> wrote:
> > > 
> > > Hi Michael,
> > > 
> > > Thanks for reviewing this on a weekend!
> > > 
> > > On 26/2/23 19:51, Michael S. Tsirkin wrote:
> > > > On Sun, Feb 26, 2023 at 01:58:10AM +0000, Damien Zammit wrote:
> > > > >       case 0:
> > > > > -        out = (d >= s->count);
> > > > > -        break;
> > > > 
> > > > 
> > > > I think you need something like
> > > >       /* FALLTHRU */
> > > > here otherwise some gcc versions will warn.
> > > > 
> > > > >       case 1:
> > > > > -        out = (d < s->count);
> > > > > +        out = (d >= s->count);
> > > 
> > > It seems that there are quite a number of these consecutive fallthrough cases
> > > without /* FALLTHRU */ in i8254_common.c
> > > 
> > > Can these be fixed in a separate patch?
> > 
> > I believe that the comment is only needed when there's code
> > between the labels and is not needed between the labels that
> > follow each other.
> 
> I think so too, I have some of these consecutive case labels in my code and
> never had a problem with that. Only when you have a statement between labels
> without break is when a comment is needed.
> 
> Regards,
> BALATON Zoltan


I just tried and it looks like you are right. Pls ignore sorry about the
noise.

-- 
MST

Re: [PATCH qemu] timer/i8254: Fix one shot PIT mode

[Michael Tokarev]

26.02.2023 04:58, Damien Zammit wrote:
> Currently, the one-shot (mode 1) PIT expires far too quickly,
> due to the output being set under the wrong logic.
> This change fixes the one-shot PIT mode to behave similarly to mode 0.
> 
> TESTED: using the one-shot PIT mode to calibrate a local apic timer.

Has this been forgotten, or is it not needed anymore?

Thanks,

/mjt

>   hw/timer/i8254_common.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/timer/i8254_common.c b/hw/timer/i8254_common.c
> index 050875b497..9164576ca9 100644
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -52,10 +52,8 @@ int pit_get_out(PITChannelState *s, int64_t current_time)
>       switch (s->mode) {
>       default:
>       case 0:
> -        out = (d >= s->count);
> -        break;
>       case 1:
> -        out = (d < s->count);
> +        out = (d >= s->count);
>           break;
>       case 2:
>           if ((d % s->count) == 0 && d != 0) {
> --
> 2.39.0
> 
> 
>