Patchwork [v3,2/7] wdt_ib700: Don't use SoftFloat int64 type

login
register
mail settings
Submitter Andreas Färber
Date Dec. 18, 2010, 4:25 p.m.
Message ID <1292689531-18763-2-git-send-email-andreas.faerber@web.de>
Download mbox | patch
Permalink /patch/76081/
State New
Headers show

Comments

Andreas Färber - Dec. 18, 2010, 4:25 p.m.
softfloat.h's int64 type has least-width semantics,
but this doesn't seem intended here, so use plain int64_t.

v3:
* Split off.

Cc: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/wdt_ib700.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Richard W.M. Jones - Dec. 18, 2010, 4:47 p.m.
On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
> softfloat.h's int64 type has least-width semantics,
> but this doesn't seem intended here, so use plain int64_t.
> 
> v3:
> * Split off.
> 
> Cc: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  hw/wdt_ib700.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
> index b6235eb..1248464 100644
> --- a/hw/wdt_ib700.c
> +++ b/hw/wdt_ib700.c
> @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp, uint32_t addr, uint32_t data)
>          30, 28, 26, 24, 22, 20, 18, 16,
>          14, 12, 10,  8,  6,  4,  2,  0
>      };
> -    int64 timeout;
> +    int64_t timeout;
>  
>      ib700_debug("addr = %x, data = %x\n", addr, data);

The use of int64(_t) was just so that the timeout calculation in the
next two lines would not overflow:

   timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
   qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);

and from you say it does seem like it was a mistake to use int64
instead of int64_t.

ACK.

In more general terms, am I doing the timeout correctly in this code?

Rich.
Andreas Färber - Dec. 19, 2010, 12:51 p.m.
Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:

> On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
>> softfloat.h's int64 type has least-width semantics,
>> but this doesn't seem intended here, so use plain int64_t.
>>
>> v3:
>> * Split off.
>>
>> Cc: Richard W.M. Jones <rjones@redhat.com>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> ---
>> hw/wdt_ib700.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
>> index b6235eb..1248464 100644
>> --- a/hw/wdt_ib700.c
>> +++ b/hw/wdt_ib700.c
>> @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,  
>> uint32_t addr, uint32_t data)
>>        30, 28, 26, 24, 22, 20, 18, 16,
>>        14, 12, 10,  8,  6,  4,  2,  0
>>    };
>> -    int64 timeout;
>> +    int64_t timeout;
>>
>>    ib700_debug("addr = %x, data = %x\n", addr, data);
>
> The use of int64(_t) was just so that the timeout calculation in the
> next two lines would not overflow:
>
>  timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
>  qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
>
> and from you say it does seem like it was a mistake to use int64
> instead of int64_t.

int64_t should be the right choice then.

> ACK.
>
> In more general terms, am I doing the timeout correctly in this code?

Being unfamiliar with both the timer code and this device, hard to say  
for me.
You're taking the lower nibble of uint32_t data and indexing  
time_map[] with it, which contains 16 elements, so okay, upcast it to  
64-bit and multiply it to ticks. Assuming that vm_clock works in  
ticks, adding the calculated timeout for the next expiry technically  
looks good. Except for the extra space. ;)

Care to provide a formal Reviewed-by or Acked-by? I'd respin it for  
Blue with updated description.

Thanks,
Andreas
Richard W.M. Jones - Dec. 19, 2010, 2:16 p.m.
On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
> Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
> 
> >On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
> >>softfloat.h's int64 type has least-width semantics,
> >>but this doesn't seem intended here, so use plain int64_t.
> >>
> >>v3:
> >>* Split off.
> >>
> >>Cc: Richard W.M. Jones <rjones@redhat.com>
> >>Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> >>---
> >>hw/wdt_ib700.c |    2 +-
> >>1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
> >>index b6235eb..1248464 100644
> >>--- a/hw/wdt_ib700.c
> >>+++ b/hw/wdt_ib700.c
> >>@@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
> >>uint32_t addr, uint32_t data)
> >>       30, 28, 26, 24, 22, 20, 18, 16,
> >>       14, 12, 10,  8,  6,  4,  2,  0
> >>   };
> >>-    int64 timeout;
> >>+    int64_t timeout;
> >>
> >>   ib700_debug("addr = %x, data = %x\n", addr, data);
> >
> >The use of int64(_t) was just so that the timeout calculation in the
> >next two lines would not overflow:
> >
> > timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
> > qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
> >
> >and from you say it does seem like it was a mistake to use int64
> >instead of int64_t.
> 
> int64_t should be the right choice then.
> 
> >ACK.
> >
> >In more general terms, am I doing the timeout correctly in this code?
> 
> Being unfamiliar with both the timer code and this device, hard to
> say for me.
> You're taking the lower nibble of uint32_t data and indexing
> time_map[] with it, which contains 16 elements, so okay, upcast it
> to 64-bit and multiply it to ticks. Assuming that vm_clock works in
> ticks, adding the calculated timeout for the next expiry technically
> looks good. Except for the extra space. ;)
> 
> Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
> Blue with updated description.

Is it enough just to write this:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Acked-by: Richard W.M. Jones <rjones@redhat.com>

or do you want me to send the updated patch with this added?

Rich.
Andreas Färber - Dec. 19, 2010, 2:28 p.m.
Am 19.12.2010 um 15:16 schrieb Richard W.M. Jones:

> On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
>> Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
>>
>>> On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
>>>> softfloat.h's int64 type has least-width semantics,
>>>> but this doesn't seem intended here, so use plain int64_t.
>>>>
>>>> v3:
>>>> * Split off.
>>>>
>>>> Cc: Richard W.M. Jones <rjones@redhat.com>
>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>> ---
>>>> hw/wdt_ib700.c |    2 +-
>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
>>>> index b6235eb..1248464 100644
>>>> --- a/hw/wdt_ib700.c
>>>> +++ b/hw/wdt_ib700.c
>>>> @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
>>>> uint32_t addr, uint32_t data)
>>>>      30, 28, 26, 24, 22, 20, 18, 16,
>>>>      14, 12, 10,  8,  6,  4,  2,  0
>>>>  };
>>>> -    int64 timeout;
>>>> +    int64_t timeout;
>>>>
>>>>  ib700_debug("addr = %x, data = %x\n", addr, data);
>>>
>>> The use of int64(_t) was just so that the timeout calculation in the
>>> next two lines would not overflow:
>>>
>>> timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
>>> qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
>>>
>>> and from you say it does seem like it was a mistake to use int64
>>> instead of int64_t.
>>
>> int64_t should be the right choice then.
>>
>>> ACK.
>>>
>>> In more general terms, am I doing the timeout correctly in this  
>>> code?
>>
>> Being unfamiliar with both the timer code and this device, hard to
>> say for me.
>> You're taking the lower nibble of uint32_t data and indexing
>> time_map[] with it, which contains 16 elements, so okay, upcast it
>> to 64-bit and multiply it to ticks. Assuming that vm_clock works in
>> ticks, adding the calculated timeout for the next expiry technically
>> looks good. Except for the extra space. ;)
>>
>> Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
>> Blue with updated description.
>
> Is it enough just to write this:
>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> Acked-by: Richard W.M. Jones <rjones@redhat.com>
>
> or do you want me to send the updated patch with this added?

Thanks, that's sufficient! I think of Acked-by as a superset of  
Reviewed-by so I'll go with the former.

Andreas
Blue Swirl - Dec. 19, 2010, 2:38 p.m.
On Sun, Dec 19, 2010 at 2:28 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 19.12.2010 um 15:16 schrieb Richard W.M. Jones:
>
>> On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
>>>
>>> Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
>>>
>>>> On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
>>>>>
>>>>> softfloat.h's int64 type has least-width semantics,
>>>>> but this doesn't seem intended here, so use plain int64_t.
>>>>>
>>>>> v3:
>>>>> * Split off.
>>>>>
>>>>> Cc: Richard W.M. Jones <rjones@redhat.com>
>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>> ---
>>>>> hw/wdt_ib700.c |    2 +-
>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
>>>>> index b6235eb..1248464 100644
>>>>> --- a/hw/wdt_ib700.c
>>>>> +++ b/hw/wdt_ib700.c
>>>>> @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
>>>>> uint32_t addr, uint32_t data)
>>>>>     30, 28, 26, 24, 22, 20, 18, 16,
>>>>>     14, 12, 10,  8,  6,  4,  2,  0
>>>>>  };
>>>>> -    int64 timeout;
>>>>> +    int64_t timeout;
>>>>>
>>>>>  ib700_debug("addr = %x, data = %x\n", addr, data);
>>>>
>>>> The use of int64(_t) was just so that the timeout calculation in the
>>>> next two lines would not overflow:
>>>>
>>>> timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
>>>> qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
>>>>
>>>> and from you say it does seem like it was a mistake to use int64
>>>> instead of int64_t.
>>>
>>> int64_t should be the right choice then.
>>>
>>>> ACK.
>>>>
>>>> In more general terms, am I doing the timeout correctly in this code?
>>>
>>> Being unfamiliar with both the timer code and this device, hard to
>>> say for me.
>>> You're taking the lower nibble of uint32_t data and indexing
>>> time_map[] with it, which contains 16 elements, so okay, upcast it
>>> to 64-bit and multiply it to ticks. Assuming that vm_clock works in
>>> ticks, adding the calculated timeout for the next expiry technically
>>> looks good. Except for the extra space. ;)
>>>
>>> Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
>>> Blue with updated description.
>>
>> Is it enough just to write this:
>>
>> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>> Acked-by: Richard W.M. Jones <rjones@redhat.com>
>>
>> or do you want me to send the updated patch with this added?
>
> Thanks, that's sufficient! I think of Acked-by as a superset of Reviewed-by
> so I'll go with the former.

No, Acked-by tag does not mean much but Reviewed-by carries a lot of weight:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=HEAD#l423
Andreas Färber - Dec. 19, 2010, 3:07 p.m.
Am 19.12.2010 um 15:38 schrieb Blue Swirl:

> On Sun, Dec 19, 2010 at 2:28 PM, Andreas Färber <andreas.faerber@web.de 
> > wrote:
>> Am 19.12.2010 um 15:16 schrieb Richard W.M. Jones:
>>
>>> On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
>>>>
>>>> Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
>>>>
>>>>> On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
>>>>>>
>>>>>> softfloat.h's int64 type has least-width semantics,
>>>>>> but this doesn't seem intended here, so use plain int64_t.
>>>>>>
>>>>>> v3:
>>>>>> * Split off.
>>>>>>
>>>>>> Cc: Richard W.M. Jones <rjones@redhat.com>
>>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>>> ---
>>>>>> hw/wdt_ib700.c |    2 +-
>>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
>>>>>> index b6235eb..1248464 100644
>>>>>> --- a/hw/wdt_ib700.c
>>>>>> +++ b/hw/wdt_ib700.c
>>>>>> @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
>>>>>> uint32_t addr, uint32_t data)
>>>>>>    30, 28, 26, 24, 22, 20, 18, 16,
>>>>>>    14, 12, 10,  8,  6,  4,  2,  0
>>>>>> };
>>>>>> -    int64 timeout;
>>>>>> +    int64_t timeout;
>>>>>>
>>>>>> ib700_debug("addr = %x, data = %x\n", addr, data);
>>>>>
>>>>> The use of int64(_t) was just so that the timeout calculation in  
>>>>> the
>>>>> next two lines would not overflow:
>>>>>
>>>>> timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
>>>>> qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
>>>>>
>>>>> and from you say it does seem like it was a mistake to use int64
>>>>> instead of int64_t.
>>>>
>>>> int64_t should be the right choice then.
>>>>
>>>>> ACK.
>>>>>
>>>>> In more general terms, am I doing the timeout correctly in this  
>>>>> code?
>>>>
>>>> Being unfamiliar with both the timer code and this device, hard to
>>>> say for me.
>>>> You're taking the lower nibble of uint32_t data and indexing
>>>> time_map[] with it, which contains 16 elements, so okay, upcast it
>>>> to 64-bit and multiply it to ticks. Assuming that vm_clock works in
>>>> ticks, adding the calculated timeout for the next expiry  
>>>> technically
>>>> looks good. Except for the extra space. ;)
>>>>
>>>> Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
>>>> Blue with updated description.
>>>
>>> Is it enough just to write this:
>>>
>>> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>>> Acked-by: Richard W.M. Jones <rjones@redhat.com>
>>>
>>> or do you want me to send the updated patch with this added?
>>
>> Thanks, that's sufficient! I think of Acked-by as a superset of  
>> Reviewed-by
>> so I'll go with the former.
>
> No, Acked-by tag does not mean much but Reviewed-by carries a lot of  
> weight:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=HEAD#l423

So, should I add both or just Reviewed-by or relieve him of the  
weight? :)
Blue Swirl - Dec. 19, 2010, 3:17 p.m.
On Sun, Dec 19, 2010 at 3:07 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 19.12.2010 um 15:38 schrieb Blue Swirl:
>
>> On Sun, Dec 19, 2010 at 2:28 PM, Andreas Färber <andreas.faerber@web.de>
>> wrote:
>>>
>>> Am 19.12.2010 um 15:16 schrieb Richard W.M. Jones:
>>>
>>>> On Sun, Dec 19, 2010 at 01:51:22PM +0100, Andreas Färber wrote:
>>>>>
>>>>> Am 18.12.2010 um 17:47 schrieb Richard W.M. Jones:
>>>>>
>>>>>> On Sat, Dec 18, 2010 at 05:25:26PM +0100, Andreas Färber wrote:
>>>>>>>
>>>>>>> softfloat.h's int64 type has least-width semantics,
>>>>>>> but this doesn't seem intended here, so use plain int64_t.
>>>>>>>
>>>>>>> v3:
>>>>>>> * Split off.
>>>>>>>
>>>>>>> Cc: Richard W.M. Jones <rjones@redhat.com>
>>>>>>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>>>>>>> ---
>>>>>>> hw/wdt_ib700.c |    2 +-
>>>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
>>>>>>> index b6235eb..1248464 100644
>>>>>>> --- a/hw/wdt_ib700.c
>>>>>>> +++ b/hw/wdt_ib700.c
>>>>>>> @@ -53,7 +53,7 @@ static void ib700_write_enable_reg(void *vp,
>>>>>>> uint32_t addr, uint32_t data)
>>>>>>>   30, 28, 26, 24, 22, 20, 18, 16,
>>>>>>>   14, 12, 10,  8,  6,  4,  2,  0
>>>>>>> };
>>>>>>> -    int64 timeout;
>>>>>>> +    int64_t timeout;
>>>>>>>
>>>>>>> ib700_debug("addr = %x, data = %x\n", addr, data);
>>>>>>
>>>>>> The use of int64(_t) was just so that the timeout calculation in the
>>>>>> next two lines would not overflow:
>>>>>>
>>>>>> timeout = (int64_t) time_map[data & 0xF] * get_ticks_per_sec();
>>>>>> qemu_mod_timer(s->timer, qemu_get_clock (vm_clock) + timeout);
>>>>>>
>>>>>> and from you say it does seem like it was a mistake to use int64
>>>>>> instead of int64_t.
>>>>>
>>>>> int64_t should be the right choice then.
>>>>>
>>>>>> ACK.
>>>>>>
>>>>>> In more general terms, am I doing the timeout correctly in this code?
>>>>>
>>>>> Being unfamiliar with both the timer code and this device, hard to
>>>>> say for me.
>>>>> You're taking the lower nibble of uint32_t data and indexing
>>>>> time_map[] with it, which contains 16 elements, so okay, upcast it
>>>>> to 64-bit and multiply it to ticks. Assuming that vm_clock works in
>>>>> ticks, adding the calculated timeout for the next expiry technically
>>>>> looks good. Except for the extra space. ;)
>>>>>
>>>>> Care to provide a formal Reviewed-by or Acked-by? I'd respin it for
>>>>> Blue with updated description.
>>>>
>>>> Is it enough just to write this:
>>>>
>>>> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>>>> Acked-by: Richard W.M. Jones <rjones@redhat.com>
>>>>
>>>> or do you want me to send the updated patch with this added?
>>>
>>> Thanks, that's sufficient! I think of Acked-by as a superset of
>>> Reviewed-by
>>> so I'll go with the former.
>>
>> No, Acked-by tag does not mean much but Reviewed-by carries a lot of
>> weight:
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=HEAD#l423
>
> So, should I add both or just Reviewed-by or relieve him of the weight? :)

As you wish ;-) I'd suppose that in theory, a patch submitter could
choose to ignore tags offered by untrustworthy reviewers or ackers,
though I don't see much advantage for doing that since they are
sharing some responsibility for the patch.

Patch

diff --git a/hw/wdt_ib700.c b/hw/wdt_ib700.c
index b6235eb..1248464 100644
--- a/hw/wdt_ib700.c
+++ b/hw/wdt_ib700.c
@@ -53,7 +53,7 @@  static void ib700_write_enable_reg(void *vp, uint32_t addr, uint32_t data)
         30, 28, 26, 24, 22, 20, 18, 16,
         14, 12, 10,  8,  6,  4,  2,  0
     };
-    int64 timeout;
+    int64_t timeout;
 
     ib700_debug("addr = %x, data = %x\n", addr, data);