Patchwork fix BCD mask for date (Solaris 2.5 guest hang fix)

login
register
mail settings
Submitter Artyom Tarasenko
Date April 23, 2012, 2:48 p.m.
Message ID <680819a0427eb4e47b78c2b7c48a699d199745c6.1335191489.git.atar4qemu@gmail.com>
Download mbox | patch
Permalink /patch/154466/
State New
Headers show

Comments

Artyom Tarasenko - April 23, 2012, 2:48 p.m.
Fix BCD mask for date. The most visible effect of this patch is
Solaris 2.5.1 doesn't hang at boot if the day of month is >21.

Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
---
 hw/m48t59.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Andreas Färber - April 23, 2012, 3:18 p.m.
Am 23.04.2012 16:48, schrieb Artyom Tarasenko:
> Fix BCD mask for date. The most visible effect of this patch is
> Solaris 2.5.1 doesn't hang at boot if the day of month is >21.
> 
> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  hw/m48t59.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

Blue has just added tests/m48t59-test.c - care to add a test case for
this? Ideally the patch would also indicate that it's about "m48t59: ".

Andreas

> diff --git a/hw/m48t59.c b/hw/m48t59.c
> index 60bbb00..0c50f45 100644
> --- a/hw/m48t59.c
> +++ b/hw/m48t59.c
> @@ -239,7 +239,7 @@ void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
>          break;
>      case 0x1FF5:
>          /* alarm date */
> -        tmp = from_bcd(val & 0x1F);
> +        tmp = from_bcd(val & 0x3F);
>          if (tmp != 0) {
>              NVRAM->alarm.tm_mday = tmp;
>              NVRAM->buffer[0x1FF5] = val;
> @@ -310,8 +310,8 @@ void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
>          break;
>      case 0x1FFD:
>      case 0x07FD:
> -        /* date */
> -	tmp = from_bcd(val & 0x1F);
> +        /* date (BCD) */
> +       tmp = from_bcd(val & 0x3F);
>  	if (tmp != 0) {
>  	    get_time(NVRAM, &tm);
>  	    tm.tm_mday = tmp;
Artyom Tarasenko - April 23, 2012, 4:48 p.m.
On Mon, Apr 23, 2012 at 5:18 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.04.2012 16:48, schrieb Artyom Tarasenko:
>> Fix BCD mask for date. The most visible effect of this patch is
>> Solaris 2.5.1 doesn't hang at boot if the day of month is >21.
>>
>> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
>> ---
>>  hw/m48t59.c |    6 +++---
>>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> Blue has just added tests/m48t59-test.c - care to add a test case for
> this?

It looks like BCD math is already covered in the "check_time" test.
I'm not sure though how m48t59 is initialized there.

>Ideally the patch would also indicate that it's about "m48t59: ".

You mean, like "[PATCH,m48t59] fix BCD mask for date (Solaris 2.5
guest hang fix)" ?

Can resend if it's necessary.

Artyom

>
> Andreas
>
>> diff --git a/hw/m48t59.c b/hw/m48t59.c
>> index 60bbb00..0c50f45 100644
>> --- a/hw/m48t59.c
>> +++ b/hw/m48t59.c
>> @@ -239,7 +239,7 @@ void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
>>          break;
>>      case 0x1FF5:
>>          /* alarm date */
>> -        tmp = from_bcd(val & 0x1F);
>> +        tmp = from_bcd(val & 0x3F);
>>          if (tmp != 0) {
>>              NVRAM->alarm.tm_mday = tmp;
>>              NVRAM->buffer[0x1FF5] = val;
>> @@ -310,8 +310,8 @@ void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
>>          break;
>>      case 0x1FFD:
>>      case 0x07FD:
>> -        /* date */
>> -     tmp = from_bcd(val & 0x1F);
>> +        /* date (BCD) */
>> +       tmp = from_bcd(val & 0x3F);
>>       if (tmp != 0) {
>>           get_time(NVRAM, &tm);
>>           tm.tm_mday = tmp;
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Blue Swirl - April 23, 2012, 5:02 p.m.
On Mon, Apr 23, 2012 at 14:48, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
> Fix BCD mask for date. The most visible effect of this patch is
> Solaris 2.5.1 doesn't hang at boot if the day of month is >21.

Thanks, applied.

>
> Signed-off-by: Artyom Tarasenko <atar4qemu@gmail.com>
> ---
>  hw/m48t59.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/m48t59.c b/hw/m48t59.c
> index 60bbb00..0c50f45 100644
> --- a/hw/m48t59.c
> +++ b/hw/m48t59.c
> @@ -239,7 +239,7 @@ void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
>         break;
>     case 0x1FF5:
>         /* alarm date */
> -        tmp = from_bcd(val & 0x1F);
> +        tmp = from_bcd(val & 0x3F);
>         if (tmp != 0) {
>             NVRAM->alarm.tm_mday = tmp;
>             NVRAM->buffer[0x1FF5] = val;
> @@ -310,8 +310,8 @@ void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
>         break;
>     case 0x1FFD:
>     case 0x07FD:
> -        /* date */
> -       tmp = from_bcd(val & 0x1F);
> +        /* date (BCD) */
> +       tmp = from_bcd(val & 0x3F);
>        if (tmp != 0) {
>            get_time(NVRAM, &tm);
>            tm.tm_mday = tmp;
> --
> 1.7.1
>
Andreas Färber - April 23, 2012, 5:34 p.m.
Am 23.04.2012 18:48, schrieb Artyom Tarasenko:
> On Mon, Apr 23, 2012 at 5:18 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Ideally the patch would also indicate that it's about "m48t59: ".
> 
> You mean, like "[PATCH,m48t59] fix BCD mask for date (Solaris 2.5
> guest hang fix)" ?

No, within [] it's not preserved for git. I meant like
"[PATCH] m48t59: Fix BCD mask for date" or
"[PATCH] hw/m48t59: Fix BCD mask for date" or something like that.

Makes it easier to spot patches on the list that are of interest - in
this case m48t59 affects ppc/PReP too, not just Solaris.

> Can resend if it's necessary.

If you don't change the code that's up to the maintainer, Blue can
probably fix it while adding *-by lines.

Andreas
Andreas Färber - April 23, 2012, 5:53 p.m.
Am 23.04.2012 19:02, schrieb Blue Swirl:
> On Mon, Apr 23, 2012 at 14:48, Artyom Tarasenko <atar4qemu@gmail.com> wrote:
>> Fix BCD mask for date. The most visible effect of this patch is
>> Solaris 2.5.1 doesn't hang at boot if the day of month is >21.
> 
> Thanks, applied.

Please take review comments you're being cc'ed on into account before
applying or, even better, fix up inconclusive subjects on your own like
Stefan does for qemu-trivial. That helps seeing from a list of commits
what subsystems they touch, esp. when backporting fixes to stable trees.

Thanks,

Andreas
Artyom Tarasenko - April 23, 2012, 6:28 p.m.
On Mon, Apr 23, 2012 at 7:34 PM, Andreas Färber <afaerber@suse.de> wrote:
> Am 23.04.2012 18:48, schrieb Artyom Tarasenko:
>> On Mon, Apr 23, 2012 at 5:18 PM, Andreas Färber <afaerber@suse.de> wrote:
>>> Ideally the patch would also indicate that it's about "m48t59: ".
>>
>> You mean, like "[PATCH,m48t59] fix BCD mask for date (Solaris 2.5
>> guest hang fix)" ?
>
> No, within [] it's not preserved for git. I meant like
> "[PATCH] m48t59: Fix BCD mask for date" or
> "[PATCH] hw/m48t59: Fix BCD mask for date" or something like that.
>
> Makes it easier to spot patches on the list that are of interest - in
> this case m48t59 affects ppc/PReP too, not just Solaris.

Right. But on the other hand, it makes obvious the use case it's
fixing. If you discover that the patch breaks PReP, and the code needs
to be changed, you'll immediately know what use case might be affected
by the change.

Also, what we currently have:
 git log --pretty=short hw/m48t59.c
shows that most commits (including the two previous ones) don't have
the m48t59 prefix.
Maybe we need a new coding convention for this?

Artyom
Andreas Färber - April 23, 2012, 6:54 p.m.
Am 23.04.2012 20:28, schrieb Artyom Tarasenko:
> On Mon, Apr 23, 2012 at 7:34 PM, Andreas Färber <afaerber@suse.de> wrote:
>> Am 23.04.2012 18:48, schrieb Artyom Tarasenko:
>>> On Mon, Apr 23, 2012 at 5:18 PM, Andreas Färber <afaerber@suse.de> wrote:
>>>> Ideally the patch would also indicate that it's about "m48t59: ".
>>>
>>> You mean, like "[PATCH,m48t59] fix BCD mask for date (Solaris 2.5
>>> guest hang fix)" ?
>>
>> No, within [] it's not preserved for git. I meant like
>> "[PATCH] m48t59: Fix BCD mask for date" or
>> "[PATCH] hw/m48t59: Fix BCD mask for date" or something like that.
>>
>> Makes it easier to spot patches on the list that are of interest - in
>> this case m48t59 affects ppc/PReP too, not just Solaris.
> 
> Right. But on the other hand, it makes obvious the use case it's
> fixing. If you discover that the patch breaks PReP, and the code needs
> to be changed, you'll immediately know what use case might be affected
> by the change.

My point was more that a) if you indicate what device it affects then we
can ignore it for supported downstream x86_64 KVM configs and b) if I
understand that it affects PReP then as maintainer I might want to
review and test it there as well *before* it gets applied and possibly
breaks something (generally speaking). m48t59 has no official maintainer
though, so any committer including Blue may commit it as long as `make
check` passes, for which we apparently don't have conclusive tests for
neither sparc nor ppc, therefore my request.

I'd personally expect consequences of a fix to go into the commit body
but that may be biased...

> Also, what we currently have:
>  git log --pretty=short hw/m48t59.c
> shows that most commits (including the two previous ones) don't have
> the m48t59 prefix.
> Maybe we need a new coding convention for this?

We have, cf. http://wiki.qemu.org/Contribute/SubmitAPatch:

Write a good commit message. QEMU follows the usual standard for git
commit messages: the first line (which becomes the email subject line)
is "subsystem: single line summary of change". Then there is a blank
line and a more detailed description of the patch, another blank and
your Signed-off-by: line. Don't include comments like "This is a
suggestion for fixing this bug" (they can go below the "---" line in the
email so they don't go into the final commit message).

My interpretation of subsystem would be "m48t59", others prefer the
exact filename "hw/m48t59.c" or something in between, but some form of
subsystem indication is expected when applicable.

If you look at qemu-devel or `git log`, most recent patches and commits
stick to the convention.

/-F

Patch

diff --git a/hw/m48t59.c b/hw/m48t59.c
index 60bbb00..0c50f45 100644
--- a/hw/m48t59.c
+++ b/hw/m48t59.c
@@ -239,7 +239,7 @@  void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
         break;
     case 0x1FF5:
         /* alarm date */
-        tmp = from_bcd(val & 0x1F);
+        tmp = from_bcd(val & 0x3F);
         if (tmp != 0) {
             NVRAM->alarm.tm_mday = tmp;
             NVRAM->buffer[0x1FF5] = val;
@@ -310,8 +310,8 @@  void m48t59_write (void *opaque, uint32_t addr, uint32_t val)
         break;
     case 0x1FFD:
     case 0x07FD:
-        /* date */
-	tmp = from_bcd(val & 0x1F);
+        /* date (BCD) */
+       tmp = from_bcd(val & 0x3F);
 	if (tmp != 0) {
 	    get_time(NVRAM, &tm);
 	    tm.tm_mday = tmp;