[PATCH] chipset_enable: Fix open /dev/cpu

[Beber]

Hi,

Here is a fix for chipset_enable.c when there is not /dev/cpu. Open
fail no there is not reason to lseek in. Actually this is a trivial fix
for not well handled return from open.

Signed-off-by: Bertrand Jacquin

Patch is attached

[ron minnich]

Acked-by: Ronald G. Minnich

[Carl-Daniel Hailfinger]

Hi Bertrand,

On 05.05.2009 22:22, Beber wrote:
> Here is a fix for chipset_enable.c when there is not /dev/cpu. Open
> fail no there is not reason to lseek in. Actually this is a trivial fix
> for not well handled return from open.

Although the fix seems trivial, it breaks the code a few lines down.
With the old code, you got a meaningful error message about having to
run "modprobe msr". This error message does not trigger anymore with the
new code.
Please fix. Thanks.

Regards,
Carl-Daniel
> Signed-off-by: Bertrand Jacquin
> Index: chipset_enable.c
>
> --- chipset_enable.c	(revision 461)
> +++ chipset_enable.c	(working copy)
> @@ -511,7 +511,7 @@
>    unsigned char buf[8];
>
> fd_msr = open("/dev/cpu/0/msr", O_RDWR);
> if (!fd_msr) {
> if (fd_msr == -1) {
> perror("open msr");
> return -1;
> }

[Myles Watson]

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]]
> On Behalf Of Carl-Daniel Hailfinger
> Sent: Tuesday, May 05, 2009 4:12 PM
> To: Beber
> Cc: [e-mail]
> Subject: Re: [coreboot] [PATCH] chipset_enable: Fix open /dev/cpu
>
> Hi Bertrand,
>
> On 05.05.2009 22:22, Beber wrote:
> > Here is a fix for chipset_enable.c when there is not /dev/cpu. Open
> > fail no there is not reason to lseek in. Actually this is a trivial fix
> > for not well handled return from open.
>
> Although the fix seems trivial, it breaks the code a few lines down.
> With the old code, you got a meaningful error message about having to
> run "modprobe msr". This error message does not trigger anymore with the
> new code.
> Please fix. Thanks.

Is this what you're asking for?

Index: util/flashrom/chipset_enable.c
===================================================================
--- util/flashrom/chipset_enable.c	(revision 462)
+++ util/flashrom/chipset_enable.c	(working copy)
@@ -512,7 +512,8 @@

fd_msr = open("/dev/cpu/0/msr", O_RDWR);
    if (fd_msr == -1) {
-		perror("open msr");
+		perror("open /dev/cpu/0/msr");
+		printf("Cannot open MSR. Did you run 'modprobe msr'?\n");
    	return -1;
    }

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 06.05.2009 00:19, Myles Watson wrote:
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]]
> > On Behalf Of Carl-Daniel Hailfinger
> > Sent: Tuesday, May 05, 2009 4:12 PM
> > To: Beber
> > Cc: [e-mail]
> > Subject: Re: [coreboot] [PATCH] chipset_enable: Fix open /dev/cpu
> >
> > Hi Bertrand,
> >
> > On 05.05.2009 22:22, Beber wrote:
> > > Here is a fix for chipset_enable.c when there is not /dev/cpu. Open
> > > fail no there is not reason to lseek in. Actually this is a trivial fix
> > > for not well handled return from open.
> >
> > Although the fix seems trivial, it breaks the code a few lines down.
> > With the old code, you got a meaningful error message about having to
> > run "modprobe msr". This error message does not trigger anymore with the
> > new code.
> > Please fix. Thanks.
>
> Is this what you're asking for?

Partially. I think it would make sense to unify this with the first
lseek() error check, but I don't have strong preferences either way.
> Index: util/flashrom/chipset_enable.c
>
> --- util/flashrom/chipset_enable.c	(revision 462)
> +++ util/flashrom/chipset_enable.c	(working copy)
> @@ -512,7 +512,8 @@
>
> fd_msr = open("/dev/cpu/0/msr", O_RDWR);
>    if (fd_msr == -1) {
> perror("open msr");
> perror("open /dev/cpu/0/msr");
> printf("Cannot open MSR. Did you run 'modprobe msr'?\n");
> return -1;
> }

If you think this is the preferred fix, I'll ack it once you signoff.

Regards,
Carl-Daniel

[Myles Watson]

On Tue, May 5, 2009 at 4:30 PM, Carl-Daniel Hailfinger
 wrote:
> On 06.05.2009 00:19, Myles Watson wrote:
> > > -----Original Message-----
> > > From: [e-mail] [mailto:[e-mail]]
> > > On Behalf Of Carl-Daniel Hailfinger
> > > Sent: Tuesday, May 05, 2009 4:12 PM
> > > To: Beber
> > > Cc: [e-mail]
> > > Subject: Re: [coreboot] [PATCH] chipset_enable: Fix open /dev/cpu
> > >
> > > Hi Bertrand,
> > >
> > > On 05.05.2009 22:22, Beber wrote:
> > > > Here is a fix for chipset_enable.c when there is not /dev/cpu. Open
> > > > fail no there is not reason to lseek in. Actually this is a trivial fix
> > > > for not well handled return from open.
> > >
> > > Although the fix seems trivial, it breaks the code a few lines down.
> > > With the old code, you got a meaningful error message about having to
> > > run "modprobe msr". This error message does not trigger anymore with the
> > > new code.
> > > Please fix. Thanks.
> >
> > Is this what you're asking for?
>
> Partially. I think it would make sense to unify this with the first
> lseek() error check, but I don't have strong preferences either way.

If you have something specific in mind, it should only be a couple of lines.

I'll ack it.

Thanks,
Myles

[Peter Stuge]

Bertrand, thanks for your patch! It is obviously needed.

Carl-Daniel Hailfinger wrote:
> Although the fix seems trivial, it breaks the code a few lines
> down.
> Please fix. Thanks.

Carl-Daniel, this is a really silly argument. Stop wasting people's
lives with such trivial things, adress it if you want to, commit,
send a thank you to the original poster, and move on!

r465

//Peter