From: Anas Nashif <nashif@...>
Subject: [PATCH] Intel Manageability Engine Interface driver
Date: Oct 22, 1:22 pm 2007
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
-
From: Andrew Morton <akpm@...>
Subject: Re: [PATCH] Intel Manageability Engine Interface driver
Date: Oct 22, 2:40 pm 2007
On Mon, 22 Oct 2007 13:22:38 -0400 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
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.
-
From: Anas Nashif <nashif@...>
Subject: Re: [PATCH] Intel Manageability Engine Interface driver
Date: Oct 23, 9:31 am 2007
Andrew,
Thanks for the feedback.
Andrew Morton wrote:
> On Mon, 22 Oct 2007 13:22:38 -0400 Anas Nashif 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
-
From: Andrew Morton <akpm@...>
Subject: Re: [PATCH] Intel Manageability Engine Interface driver
Date: Oct 23, 12:25 pm 2007
On Tue, 23 Oct 2007 09:31:07 -0400 Anas Nashif wrote:
> Andrew,
>
> Thanks for the feedback.
>
> Andrew Morton wrote:
> > On Mon, 22 Oct 2007 13:22:38 -0400 Anas Nashif 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.
-
