| Message ID | 680819a0427eb4e47b78c2b7c48a699d199745c6.1335191489.git.atar4qemu@gmail.com |
|---|---|
| State | New |
| Headers | show |
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;
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
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 >
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
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
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
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
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;
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(-)