linux-kernel - Re: [PATCH] Intel Management Engine Interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080523070452.GA8193@ucw.cz>
Date:	Fri, 23 May 2008 09:04:52 +0200
From:	Pavel Machek <pavel@....cz>
To:	Anas Nashif <nashif@...ux.intel.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Intel Management Engine Interface

Hi!

> The Intel Management Engine Interface (aka HECI: Host Embedded
> Controller Interface ) enables communication between the host OS and
> the Management Engine firmware. MEI is bi-directional, and either the
> host or Intel AMT firmware can initiate transactions.

* uses homebrewn DBG(), could it use dprintk() or something.

* should probably come with Documentation/ file

* defines like H_IE are a bit too terse.

> +extern const struct guid heci_pthi_guid;
> +extern const struct guid heci_wd_guid;
> +extern const __u8 start_wd_params[];
> +extern const __u8 stop_wd_params[];
> +extern const __u8 heci_wd_state_independence_msg[2][4];

Externs should go to header file.

No need to use __u8 in kernel.

Variables need good names. pthi_guid is not one of them.

> +/**
> + * memory IO BAR definition
> + */
> +#define     BAR_0                        0
> +#define     BAR_1                        1
> +#define     BAR_5                        5

Heh. And comments should tell us what it is.

Hmm, is this kerneldoc? Does not seem like one. So don't use /**. Fix
this all over the patch.

> +/**
> + * heci_fe_same_id - tell if file extensions have same id
> + * @fe1 -file extension
> + * @fe2 -file extension
> + *
> + * @return :
> + *  1 - if ids are the same,
> + *  0 - if differ.
> + */

File extensions?!

> +static inline int heci_fe_same_id(struct heci_file_private *fe1,
> +				  struct heci_file_private *fe2)
> +{
> +	if ((fe1->host_client_id == fe2->host_client_id)
> +	    && (fe1->me_client_id == fe2->me_client_id)) {
> +		return 1;
> +	} else {
> +		return 0;
> +	}
> +}

This can be written without the if.


> +#endif /* _HECI_H_ */
> diff --git a/drivers/char/heci/heci_data_structures.h
> b/drivers/char/heci/heci_data_structures.h
> new file mode 100644
> index 0000000..ac0f488
> --- /dev/null
> +++ b/drivers/char/heci/heci_data_structures.h
> @@ -0,0 +1,507 @@
> +/*
> + * Part of Intel(R) Manageability Engine Interface Linux driver
> + *
> + * Copyright (c) 2003 - 2008 Intel Corp.
> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions, and the following disclaimer,
> + *    without modification.
> + * 2. Redistributions in binary form must reproduce at minimum a disclaimer
> + *    substantially similar to the "NO WARRANTY" disclaimer below
> + *    ("Disclaimer") and any redistribution must be conditioned upon
> + *    including a substantially similar Disclaimer requirement for further
> + *    binary redistribution.
> + * 3. Neither the names of the above-listed copyright holders nor the names
> + *    of any contributors may be used to endorse or promote products derived
> + *    from this software without specific prior written permission.

What kind of ninja mutant license is this?

> +/**
> + * Number of queue lists used by this driver
> + */
> +#define NUMBER_OF_LISTS        7

Yes, how very useful name. Plus, it is too generic and likely to
conflict with someone else.

> +#define IAMTHIF_MTU 4160

And this one seems encrypted.

> +
> +/**
> + * HECI HW Section
> + */
> +
> +/* HECI addresses and defines */
> +#define H_CB_WW    0
> +#define H_CSR      4
> +#define ME_CB_RW   8
> +#define ME_CSR_HA  0xC

As do this and registers below.

> +/**
> + * time to wait event
> + */
> +#define HECI_INTEROP_TIMEOUT    (HZ * 7)

Could we get _useful_ comments?

> +/* File state */
> +enum file_state {
> +	HECI_FILE_INITIALIZING = 0,
> +	HECI_FILE_CONNECTING,
> +	HECI_FILE_CONNECTED,
> +	HECI_FILE_DISCONNECTING,
> +	HECI_FILE_DISCONNECTED
> +};

What kind of files is this handling? Surely this is not a filesystem?


> +/* HECI states */
> +enum heci_states{

Missing space, and evil comment.
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/