LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Intel Manageability Engine Interface driver 
@ 2007-10-22 17:22 Anas Nashif
  2007-10-22 18:28 ` Xavier Bestel
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Anas Nashif @ 2007-10-22 17:22 UTC (permalink / raw)
  To: linux-kernel; +Cc: Marek Dabek, Andrew Morton

The Manageability Engine Interface (aka HECI) allows applications to 
communicate with the Intel(R) Manageability Engine (ME) firmware.

It is meant to be used by user-space manageability applications to
access ME features such as Intel(R) Active Management Technology,
Intel(R) Quiet System Technology and ASF.

The patch is large so I'm placing the diff on the web for download
rather than attaching it here.  Download:

http://download.openamt.org/intel-MEI.diff


Anas

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 17:22 [PATCH] Intel Manageability Engine Interface driver Anas Nashif
@ 2007-10-22 18:28 ` Xavier Bestel
  2007-10-22 18:40 ` Andrew Morton
  2007-10-22 22:30 ` Maxim Levitsky
  2 siblings, 0 replies; 17+ messages in thread
From: Xavier Bestel @ 2007-10-22 18:28 UTC (permalink / raw)
  To: Anas Nashif; +Cc: linux-kernel, Marek Dabek, Andrew Morton

Hi,

Le lundi 22 octobre 2007 à 13:22 -0400, Anas Nashif a écrit :
> The Manageability Engine Interface (aka HECI) allows applications to 
> communicate with the Intel(R) Manageability Engine (ME) firmware.
> 
> It is meant to be used by user-space manageability applications to
> access ME features such as Intel(R) Active Management Technology,
> Intel(R) Quiet System Technology and ASF.

Could you briefly explain all these terms ?

Thanks,
	Xav



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 17:22 [PATCH] Intel Manageability Engine Interface driver Anas Nashif
  2007-10-22 18:28 ` Xavier Bestel
@ 2007-10-22 18:40 ` Andrew Morton
  2007-10-23 13:31   ` Anas Nashif
  2007-10-23 15:04   ` Lennart Sorensen
  2007-10-22 22:30 ` Maxim Levitsky
  2 siblings, 2 replies; 17+ messages in thread
From: Andrew Morton @ 2007-10-22 18:40 UTC (permalink / raw)
  To: Anas Nashif; +Cc: linux-kernel, Marek Dabek

On Mon, 22 Oct 2007 13:22:38 -0400 Anas Nashif <nashif@linux.intel.com> wrote:

> The Manageability Engine Interface (aka HECI) allows applications to 
> communicate with the Intel(R) Manageability Engine (ME) firmware.
> 
> It is meant to be used by user-space manageability applications to
> access ME features such as Intel(R) Active Management Technology,
> Intel(R) Quiet System Technology and ASF.
> 
> The patch is large so I'm placing the diff on the web for download
> rather than attaching it here.  Download:
> 
> http://download.openamt.org/intel-MEI.diff

Please get the patches mailed out somehow.  Consider splitting the patch
up.  I think you could send it as-is (200k will fit, I believe) but it's
rather too large to review effectively.

The code looks good from a quick scan.  Immediate impressions from a
quick scan, mainly trivia:



- Consider using open-coded "0" in place of ESUCCESS

- Remove #pragme pack(1), use __attribute__((packed))

- Why does a new driver have "additional char device for legacy mode"?

- Remove the INFO(), ERR() and WARN() macros, just open-code the printk calls

- Remove the commented-out DEF_PARM, HECI_DEBUG, any others

- You can't have a global variable called "debug"!

- Remove private definitions of TRUE and FALSE.  Use standard "true" and
  "false" or just open-code 0 and 1.

- This:

+     /**
+      * virtual void GetParam(const char* UserParam);
+      * read write messages to/from heci fw
+      */

  appears to be dead code, should be called get_param, should use
  tabstops not spaces

- These:

+#define SHUTDOWN_METHOD(method) .shutdown = method,
+#define HECI_REBOOT_NOTIFIER(reboot_notifier, driver, reboot_function)
+#define REGISTER_REBOOT_NOTIFIER(reboot_notifier)
+#define UNREGISTER_REBOOT_NOTIFIER(reboot_notifier)
+#define heci_reboot_notifier

  will be unpopular.  See if they can be removed via suitable means?

- heci_init.c has these:

+/**
+ *  heci init function prototypes
+ */
+int host_start_message(struct iamt_heci_device *dev);
+int host_enum_clients_message(struct iamt_heci_device *dev);
+int allocate_me_clents_storage(struct iamt_heci_device *dev);
+void heci_disable(struct iamt_heci_device *dev);
+void host_init_wd(struct iamt_heci_device *dev);
+void host_init_legacy(struct iamt_heci_device *dev);
+int heci_wait_event_int_timeout(struct iamt_heci_device *dev, long timeout);

  which are extern-declarations-in-C which checkpatch failed to detect. 
  They should be placed in a header file or just removed.

- please review all global symbols, check that they really do need to be
  global, and that they have suitable names (ie: not like "debug")

- mysterious msleep(100) in host_start_message()?  Needs a comment.

- Please prefer to put a blank line between the declaration of locals and
  the start of code in each function.

- Here:

+		if (0 == memcmp(&heci_wd_guid,

  we boringly prefer "if (foo == 0)" rather than "if (0 == foo)".  (lots
  of places).

- The changelog could do with some expansion.  What is "Intel(R)
  Manageability Engine (ME) firmware"?  Why do we want to include this code
  in Linux?  What value has it to our users, etc?  Bascially: tell us more
  stuff.



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 17:22 [PATCH] Intel Manageability Engine Interface driver Anas Nashif
  2007-10-22 18:28 ` Xavier Bestel
  2007-10-22 18:40 ` Andrew Morton
@ 2007-10-22 22:30 ` Maxim Levitsky
  2007-10-23 13:35   ` Anas Nashif
  2007-10-25  9:33   ` Gerd Hoffmann
  2 siblings, 2 replies; 17+ messages in thread
From: Maxim Levitsky @ 2007-10-22 22:30 UTC (permalink / raw)
  To: Anas Nashif; +Cc: linux-kernel, Marek Dabek, Andrew Morton

On Monday 22 October 2007 19:22:38 Anas Nashif wrote:
> The Manageability Engine Interface (aka HECI) allows applications to 
> communicate with the Intel(R) Manageability Engine (ME) firmware.
> 
> It is meant to be used by user-space manageability applications to
> access ME features such as Intel(R) Active Management Technology,
> Intel(R) Quiet System Technology and ASF.
> 
> The patch is large so I'm placing the diff on the web for download
> rather than attaching it here.  Download:
> 
> http://download.openamt.org/intel-MEI.diff
> 
> 
> Anas
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


Will integrated sensors on DG965 motherboard be supported?
They can be accessed only through the
HECI interface, but the board has no AMT, and even if it had one, I don't 
think AMT includes HW sensors.

Will it be supported?
I really need it (I have to reboot to bios each time to see the sensors, which is
both inaccurate, and annoying)


Best regards,
	Maxim Levitsky

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 18:40 ` Andrew Morton
@ 2007-10-23 13:31   ` Anas Nashif
  2007-10-23 13:36     ` Maxim Levitsky
  2007-10-23 16:25     ` Andrew Morton
  2007-10-23 15:04   ` Lennart Sorensen
  1 sibling, 2 replies; 17+ messages in thread
From: Anas Nashif @ 2007-10-23 13:31 UTC (permalink / raw)
  To: Andrew Morton; +Cc: linux-kernel, Marek Dabek

Andrew,

Thanks for the feedback.

Andrew Morton wrote:
> On Mon, 22 Oct 2007 13:22:38 -0400 Anas Nashif <nashif@linux.intel.com> wrote:
>>
>> The patch is large so I'm placing the diff on the web for download
>> rather than attaching it here.  Download:
>>
>> http://download.openamt.org/intel-MEI.diff
> 
> Please get the patches mailed out somehow.  Consider splitting the patch
> up.  I think you could send it as-is (200k will fit, I believe) but it's
> rather too large to review effectively.
> 
> The code looks good from a quick scan.  Immediate impressions from a
> quick scan, mainly trivia:
> 

We will fix the issues below and send the revised patch to the list.

>
> 
> - Why does a new driver have "additional char device for legacy mode"?
> 

It is not quite new. What is currently considered legacy was supported 
on Linux with a driver that was never submitted upstream (although it is 
open-source and available from e1000.sf.net).
Some applications still use the legacy interface (KCS style) and have to 
be supported with the new driver as well.

(...)

> - The changelog could do with some expansion.  What is "Intel(R)
>   Manageability Engine (ME) firmware"?  Why do we want to include this code
>   in Linux?  What value has it to our users, etc?  Bascially: tell us more
>   stuff.

The core hardware architecture of Intel Active Management Technology 
(Intel AMT) is resident in firmware. The micro-controller within the 
chipset's graphics and memory controller (MCH) hub houses the Management 
Engine (ME) firmware, which implements various services on behalf of 
management applications. Additionally, flash memory houses system BIOS, 
code used by the management engine, and a third-party data store (3PDS) 
that enables applications to store information as needed in non-volatile 
memory.

Communication between the host OS and the ME is accomplished by means of 
the Intel Management Engine Interface (aka HECI: Host Embedded 
Controller Interface ). MEI is bi-directional, and either the host or 
Intel AMT firmware can initiate transactions.

Some of the ME subsystems that can be access via MEI driver:

- Intel(R) Quiet System Technology (QST) is implemented as a firmware 
subsystem  that  runs in the ME.  Programs that wish to expose the 
health monitoring and fan speed control capabilities of Intel(R) QST 
will need to use the MEI driver to communicate with the ME sub-system.
- ASF is the "Alert Standard Format" which is an DMTF manageability 
standard. It is implemented in the PC's hardware and firmware, and is 
managed from a remote console.

Most recent Intel desktop chipsets have one or more of the above ME 
services. The MEI driver will make it possible to support the above 
features on Linux and provides applications access to the ME and it's 
features. The MEI drivers will also help bridge a current gap related to 
lm_sensors support on recent desktop chipsets.


Anas



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 22:30 ` Maxim Levitsky
@ 2007-10-23 13:35   ` Anas Nashif
  2007-10-23 13:42     ` Maxim Levitsky
  2007-10-25  9:33   ` Gerd Hoffmann
  1 sibling, 1 reply; 17+ messages in thread
From: Anas Nashif @ 2007-10-23 13:35 UTC (permalink / raw)
  To: Maxim Levitsky; +Cc: linux-kernel, Marek Dabek, Andrew Morton

Hi,

Maxim Levitsky wrote:
> 
> Will integrated sensors on DG965 motherboard be supported?
> They can be accessed only through the
> HECI interface, but the board has no AMT, and even if it had one, I don't 
> think AMT includes HW sensors.
> 
True, this is supported using QST.

> Will it be supported?

The MEI driver (HECI) is the first step toward supporting this in Linux. 
We are working on a QST release for Linux in the form of an SDK that can 
be used for example by lm_sensors to expose the sensors.

> I really need it (I have to reboot to bios each time to see the sensors, which is
> both inaccurate, and annoying)
> 

Yes, we know how annoying it is and we are working on it.

Anas

> 
> Best regards,
> 	Maxim Levitsky

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 13:31   ` Anas Nashif
@ 2007-10-23 13:36     ` Maxim Levitsky
  2007-10-23 16:25     ` Andrew Morton
  1 sibling, 0 replies; 17+ messages in thread
From: Maxim Levitsky @ 2007-10-23 13:36 UTC (permalink / raw)
  To: Anas Nashif; +Cc: Andrew Morton, linux-kernel, Marek Dabek

On Tuesday 23 October 2007 15:31:07 Anas Nashif wrote:
> Andrew,
> 
> Thanks for the feedback.
> 
> Andrew Morton wrote:
> > On Mon, 22 Oct 2007 13:22:38 -0400 Anas Nashif <nashif@linux.intel.com> wrote:
> >>
> >> The patch is large so I'm placing the diff on the web for download
> >> rather than attaching it here.  Download:
> >>
> >> http://download.openamt.org/intel-MEI.diff
> > 
> > Please get the patches mailed out somehow.  Consider splitting the patch
> > up.  I think you could send it as-is (200k will fit, I believe) but it's
> > rather too large to review effectively.
> > 
> > The code looks good from a quick scan.  Immediate impressions from a
> > quick scan, mainly trivia:
> > 
> 
> We will fix the issues below and send the revised patch to the list.
> 
> >
> > 
> > - Why does a new driver have "additional char device for legacy mode"?
> > 
> 
> It is not quite new. What is currently considered legacy was supported 
> on Linux with a driver that was never submitted upstream (although it is 
> open-source and available from e1000.sf.net).
> Some applications still use the legacy interface (KCS style) and have to 
> be supported with the new driver as well.
> 
> (...)
> 
> > - The changelog could do with some expansion.  What is "Intel(R)
> >   Manageability Engine (ME) firmware"?  Why do we want to include this code
> >   in Linux?  What value has it to our users, etc?  Bascially: tell us more
> >   stuff.
> 
> The core hardware architecture of Intel Active Management Technology 
> (Intel AMT) is resident in firmware. The micro-controller within the 
> chipset's graphics and memory controller (MCH) hub houses the Management 
> Engine (ME) firmware, which implements various services on behalf of 
> management applications. Additionally, flash memory houses system BIOS, 
> code used by the management engine, and a third-party data store (3PDS) 
> that enables applications to store information as needed in non-volatile 
> memory.
> 
> Communication between the host OS and the ME is accomplished by means of 
> the Intel Management Engine Interface (aka HECI: Host Embedded 
> Controller Interface ). MEI is bi-directional, and either the host or 
> Intel AMT firmware can initiate transactions.
> 
> Some of the ME subsystems that can be access via MEI driver:
> 
> - Intel(R) Quiet System Technology (QST) is implemented as a firmware 
> subsystem  that  runs in the ME.  Programs that wish to expose the 
> health monitoring and fan speed control capabilities of Intel(R) QST 
> will need to use the MEI driver to communicate with the ME sub-system.
> - ASF is the "Alert Standard Format" which is an DMTF manageability 
> standard. It is implemented in the PC's hardware and firmware, and is 
> managed from a remote console.
> 
> Most recent Intel desktop chipsets have one or more of the above ME 
> services. The MEI driver will make it possible to support the above 
> features on Linux and provides applications access to the ME and it's 
> features. The MEI drivers will also help bridge a current gap related to 
> lm_sensors support on recent desktop chipsets.
Care to tell me, and other users of recent intel motherboards,
when QST tool to access HW sensors will be released, or at least
is it developed?
> 
> 
> Anas
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


Best regards,
	Maxim Levitsky

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 13:35   ` Anas Nashif
@ 2007-10-23 13:42     ` Maxim Levitsky
  0 siblings, 0 replies; 17+ messages in thread
From: Maxim Levitsky @ 2007-10-23 13:42 UTC (permalink / raw)
  To: Anas Nashif; +Cc: linux-kernel, Marek Dabek, Andrew Morton

On Tuesday 23 October 2007 15:35:37 Anas Nashif wrote:
> Hi,
> 
> Maxim Levitsky wrote:
> > 
> > Will integrated sensors on DG965 motherboard be supported?
> > They can be accessed only through the
> > HECI interface, but the board has no AMT, and even if it had one, I don't 
> > think AMT includes HW sensors.
> > 
> True, this is supported using QST.
> 
> > Will it be supported?
> 
> The MEI driver (HECI) is the first step toward supporting this in Linux. 
> We are working on a QST release for Linux in the form of an SDK that can 
> be used for example by lm_sensors to expose the sensors.\
This would be great, any dates?, at least approximate... 

> 
> > I really need it (I have to reboot to bios each time to see the sensors, which is
> > both inaccurate, and annoying)
> > 
> 
> Yes, we know how annoying it is and we are working on it.
> 
> Anas
> 
> > 
> > Best regards,
> > 	Maxim Levitsky
> 
Best regards,
	Maxim Levitsky

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 18:40 ` Andrew Morton
  2007-10-23 13:31   ` Anas Nashif
@ 2007-10-23 15:04   ` Lennart Sorensen
  2007-10-23 16:23     ` Arjan van de Ven
  2007-10-23 16:32     ` Andrew Morton
  1 sibling, 2 replies; 17+ messages in thread
From: Lennart Sorensen @ 2007-10-23 15:04 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Anas Nashif, linux-kernel, Marek Dabek

On Mon, Oct 22, 2007 at 11:40:01AM -0700, Andrew Morton wrote:
> - Here:
> 
> +		if (0 == memcmp(&heci_wd_guid,
> 
>   we boringly prefer "if (foo == 0)" rather than "if (0 == foo)".  (lots
>   of places).

But 0 == blah is safer.  If you accidentally do 0 = blah the compiler
will tell you.  Just because people have always done it the other way
around doesn't make it the right way to do it.  I have noticed many
people have started to realize this in the last few years.

It is also much clearer that you are comparing against a constant and
not doing an assignment when the constant comes before the variable.

I think to encourage people doing it the less safe way is just silly.

Some places in the kernel that already uses the constant first are:

./sound/oss/btaudio.c
./sound/usb/usx2y/
./net/sctp/
./arch/x86_64/mm/init.c
./arch/cris/arch-v32/drivers/cryptocop.c
./drivers/i2c/algos/i2c-algo-pcf.c
./drivers/mtd/devices/doc2000.c
./drivers/net/iseries_veth.c
./drivers/net/hp100.c
./drivers/net/phy/phy.c
./drivers/net/xen-netfront.c
./drivers/scsi/scsi_error.c
./drivers/scsi/sr_ioctl.c
./drivers/scsi/ch.c
./drivers/scsi/sg.c
./drivers/scsi/sr_vendor.c
./drivers/scsi/scsi_debug.c
./drivers/media/video/tea6415c.c
./drivers/media/video/tvaudio.c
./drivers/media/video/bt8xx/
./drivers/char/ip2/
./drivers/isdn/hisax/
./drivers/sbus/char/
./fs/nfsd/

and I am sure there a quite a few more.  It ought to be encouraged for
all new code to avoid stupid typo bugs that are hard to see.

--
Len Sorensen

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 15:04   ` Lennart Sorensen
@ 2007-10-23 16:23     ` Arjan van de Ven
  2007-10-23 18:05       ` Lennart Sorensen
  2007-10-23 16:32     ` Andrew Morton
  1 sibling, 1 reply; 17+ messages in thread
From: Arjan van de Ven @ 2007-10-23 16:23 UTC (permalink / raw)
  To: Lennart Sorensen; +Cc: Andrew Morton, Anas Nashif, linux-kernel, Marek Dabek

On Tue, 23 Oct 2007 11:04:29 -0400
lsorense@csclub.uwaterloo.ca (Lennart Sorensen) wrote:

> On Mon, Oct 22, 2007 at 11:40:01AM -0700, Andrew Morton wrote:
> > - Here:
> > 
> > +		if (0 == memcmp(&heci_wd_guid,
> > 
> >   we boringly prefer "if (foo == 0)" rather than "if (0 == foo)".
> > (lots of places).
> 
> But 0 == blah is safer.  If you accidentally do 0 = blah the compiler
> will tell you. 

gcc will tell you in the other direction just as well.

and people read from left to right (at least in english) so coding in
that direction is generally preferred in the Linux kernel as well.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 13:31   ` Anas Nashif
  2007-10-23 13:36     ` Maxim Levitsky
@ 2007-10-23 16:25     ` Andrew Morton
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Morton @ 2007-10-23 16:25 UTC (permalink / raw)
  To: Anas Nashif; +Cc: linux-kernel, Marek Dabek

On Tue, 23 Oct 2007 09:31:07 -0400 Anas Nashif <nashif@linux.intel.com> wrote:

> Andrew,
> 
> Thanks for the feedback.
> 
> Andrew Morton wrote:
> > On Mon, 22 Oct 2007 13:22:38 -0400 Anas Nashif <nashif@linux.intel.com> wrote:
> >>
> >> The patch is large so I'm placing the diff on the web for download
> >> rather than attaching it here.  Download:
> >>
> >> http://download.openamt.org/intel-MEI.diff
> > 
> > Please get the patches mailed out somehow.  Consider splitting the patch
> > up.  I think you could send it as-is (200k will fit, I believe) but it's
> > rather too large to review effectively.
> > 
> > The code looks good from a quick scan.  Immediate impressions from a
> > quick scan, mainly trivia:
> > 
> 
> We will fix the issues below and send the revised patch to the list.
> 
> >
> > 
> > - Why does a new driver have "additional char device for legacy mode"?
> > 
> 
> It is not quite new. What is currently considered legacy was supported 
> on Linux with a driver that was never submitted upstream (although it is 
> open-source and available from e1000.sf.net).
> Some applications still use the legacy interface (KCS style) and have to 
> be supported with the new driver as well.
> 

It would be better to remove the lecacy mode support from the new driver
and to continue to ship a patch for those people who use the old interface.
 They've been patching in the whole driver thus far so I assume all the
processes are already in place for this.

> 
> > - The changelog could do with some expansion.  What is "Intel(R)
> >   Manageability Engine (ME) firmware"?  Why do we want to include this code
> >   in Linux?  What value has it to our users, etc?  Bascially: tell us more
> >   stuff.
> 
> The core hardware architecture of Intel Active Management Technology 
> (Intel AMT) is resident in firmware. The micro-controller within the 
> chipset's graphics and memory controller (MCH) hub houses the Management 
> Engine (ME) firmware, which implements various services on behalf of 
> management applications. Additionally, flash memory houses system BIOS, 
> code used by the management engine, and a third-party data store (3PDS) 
> that enables applications to store information as needed in non-volatile 
> memory.
> 
> Communication between the host OS and the ME is accomplished by means of 
> the Intel Management Engine Interface (aka HECI: Host Embedded 
> Controller Interface ). MEI is bi-directional, and either the host or 
> Intel AMT firmware can initiate transactions.
> 
> Some of the ME subsystems that can be access via MEI driver:
> 
> - Intel(R) Quiet System Technology (QST) is implemented as a firmware 
> subsystem  that  runs in the ME.  Programs that wish to expose the 
> health monitoring and fan speed control capabilities of Intel(R) QST 
> will need to use the MEI driver to communicate with the ME sub-system.
> - ASF is the "Alert Standard Format" which is an DMTF manageability 
> standard. It is implemented in the PC's hardware and firmware, and is 
> managed from a remote console.
> 
> Most recent Intel desktop chipsets have one or more of the above ME 
> services. The MEI driver will make it possible to support the above 
> features on Linux and provides applications access to the ME and it's 
> features. The MEI drivers will also help bridge a current gap related to 
> lm_sensors support on recent desktop chipsets.
> 

I see, thanks.  That would be a fine addition to the patch's changelog,
please.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 15:04   ` Lennart Sorensen
  2007-10-23 16:23     ` Arjan van de Ven
@ 2007-10-23 16:32     ` Andrew Morton
  1 sibling, 0 replies; 17+ messages in thread
From: Andrew Morton @ 2007-10-23 16:32 UTC (permalink / raw)
  To: Lennart Sorensen; +Cc: Anas Nashif, linux-kernel, Marek Dabek

On Tue, 23 Oct 2007 11:04:29 -0400 lsorense@csclub.uwaterloo.ca (Lennart Sorensen) wrote:

> On Mon, Oct 22, 2007 at 11:40:01AM -0700, Andrew Morton wrote:
> > - Here:
> > 
> > +		if (0 == memcmp(&heci_wd_guid,
> > 
> >   we boringly prefer "if (foo == 0)" rather than "if (0 == foo)".  (lots
> >   of places).
> 
> But 0 == blah is safer.  If you accidentally do 0 = blah the compiler
> will tell you.

If you do 'if (blah = 0)' then compiler will tell you too.  To all intents
and purposes this invalidates the reasons for doing `if (0 == blah)'.

>  Just because people have always done it the other way
> around doesn't make it the right way to do it.  I have noticed many
> people have started to realize this in the last few years.
> 
> It is also much clearer that you are comparing against a constant and
> not doing an assignment when the constant comes before the variable.
> 
> I think to encourage people doing it the less safe way is just silly.

It isn't less safe.

> Some places in the kernel that already uses the constant first are:

It impacts readability.  All the aio code was implemented that way for a
few years and it drove everyone so batty that we undid it.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 16:23     ` Arjan van de Ven
@ 2007-10-23 18:05       ` Lennart Sorensen
  2007-10-23 18:22         ` Roland Dreier
  0 siblings, 1 reply; 17+ messages in thread
From: Lennart Sorensen @ 2007-10-23 18:05 UTC (permalink / raw)
  To: Arjan van de Ven; +Cc: Andrew Morton, Anas Nashif, linux-kernel, Marek Dabek

On Tue, Oct 23, 2007 at 09:23:33AM -0700, Arjan van de Ven wrote:
> gcc will tell you in the other direction just as well.
> 
> and people read from left to right (at least in english) so coding in
> that direction is generally preferred in the Linux kernel as well.

What does gcc have to say about if (foo = 0){ rather than if (foo == 0){

Both are legal C so it shouldn't say anything.  Of course the first is
usually a bug (or very bad style).

On the other hand if (0 = foo) will give an error.

It isn't about how you read in english, it is about not making mistakes.
And why can't you say if 0 is equal to the variable foo rather than if
the variable foo is equal to 0?  Both are valid english, so that is just
a crappy excuse for sticking with a bad idea.

--
Len Sorensen

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 18:05       ` Lennart Sorensen
@ 2007-10-23 18:22         ` Roland Dreier
  2007-10-23 19:35           ` Lennart Sorensen
  0 siblings, 1 reply; 17+ messages in thread
From: Roland Dreier @ 2007-10-23 18:22 UTC (permalink / raw)
  To: Lennart Sorensen
  Cc: Arjan van de Ven, Andrew Morton, Anas Nashif, linux-kernel, Marek Dabek

 > What does gcc have to say about if (foo = 0){ rather than if (foo == 0){

It's not a hard experiment to do.

The answer is:

    warning: suggest parentheses around assignment used as truth value

 - R.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 18:22         ` Roland Dreier
@ 2007-10-23 19:35           ` Lennart Sorensen
  2007-10-24  9:00             ` Bernd Petrovitsch
  0 siblings, 1 reply; 17+ messages in thread
From: Lennart Sorensen @ 2007-10-23 19:35 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Arjan van de Ven, Andrew Morton, Anas Nashif, linux-kernel, Marek Dabek

On Tue, Oct 23, 2007 at 11:22:50AM -0700, Roland Dreier wrote:
> It's not a hard experiment to do.
> 
> The answer is:
> 
>     warning: suggest parentheses around assignment used as truth value

A warning is not an error.  It won't abort the compile.

The warning (which I don't remember gcc doing in the past) is a nice
idea though.

--
Len Sorensen

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-23 19:35           ` Lennart Sorensen
@ 2007-10-24  9:00             ` Bernd Petrovitsch
  0 siblings, 0 replies; 17+ messages in thread
From: Bernd Petrovitsch @ 2007-10-24  9:00 UTC (permalink / raw)
  To: Lennart Sorensen
  Cc: Roland Dreier, Arjan van de Ven, Andrew Morton, Anas Nashif,
	linux-kernel, Marek Dabek

On Die, 2007-10-23 at 15:35 -0400, Lennart Sorensen wrote:
> On Tue, Oct 23, 2007 at 11:22:50AM -0700, Roland Dreier wrote:
> > It's not a hard experiment to do.
> > 
> > The answer is:
> > 
> >     warning: suggest parentheses around assignment used as truth value
> 
> A warning is not an error.  It won't abort the compile.

Add "-Werror".

> The warning (which I don't remember gcc doing in the past) is a nice
> idea though.

That's the case since many years - I don't remember how long.

	Bernd
-- 
Firmix Software GmbH                   http://www.firmix.at/
mobil: +43 664 4416156                 fax: +43 1 7890849-55
          Embedded Linux Development and Services



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Intel Manageability Engine Interface driver
  2007-10-22 22:30 ` Maxim Levitsky
  2007-10-23 13:35   ` Anas Nashif
@ 2007-10-25  9:33   ` Gerd Hoffmann
  1 sibling, 0 replies; 17+ messages in thread
From: Gerd Hoffmann @ 2007-10-25  9:33 UTC (permalink / raw)
  To: Maxim Levitsky; +Cc: Anas Nashif, linux-kernel, Marek Dabek, Andrew Morton

Maxim Levitsky wrote:
> Will integrated sensors on DG965 motherboard be supported?
> They can be accessed only through the
> HECI interface, but the board has no AMT, and even if it had one, I don't 
> think AMT includes HW sensors.

While talking about AMT ...

<ad>

While talking about AMT:  One quite useful thing is AMT provides is
*remote* management (most useful for kernel hackers: serial-over-lan
console, remote reset).  I've hacked up some tools to use this stuff.
Package is is called amtterm.

Source code: http://dl.bytesex.org/releases/amtterm/

Fedora 7+8 have ready-to-go packages in the repositories, so you can
just "yum install amtterm".

SuSE packages are in the buildservice:
http://download.opensuse.org/repositories/home:/kraxel/

</ad>

HTH,
  Gerd

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2007-10-25  9:33 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2007-10-22 17:22 [PATCH] Intel Manageability Engine Interface driver Anas Nashif
2007-10-22 18:28 ` Xavier Bestel
2007-10-22 18:40 ` Andrew Morton
2007-10-23 13:31   ` Anas Nashif
2007-10-23 13:36     ` Maxim Levitsky
2007-10-23 16:25     ` Andrew Morton
2007-10-23 15:04   ` Lennart Sorensen
2007-10-23 16:23     ` Arjan van de Ven
2007-10-23 18:05       ` Lennart Sorensen
2007-10-23 18:22         ` Roland Dreier
2007-10-23 19:35           ` Lennart Sorensen
2007-10-24  9:00             ` Bernd Petrovitsch
2007-10-23 16:32     ` Andrew Morton
2007-10-22 22:30 ` Maxim Levitsky
2007-10-23 13:35   ` Anas Nashif
2007-10-23 13:42     ` Maxim Levitsky
2007-10-25  9:33   ` Gerd Hoffmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).