| Message ID | 1292689531-18763-2-git-send-email-andreas.faerber@web.de |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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
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
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? :)
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.
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);
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(-)