Re: [announce][draft3] HVCS for inclusion in 2.6 tree

["Randy.Dunlap" <rddunlap@osdl.org>]

On Tue, 27 Jul 2004 15:08:58 -0500 Ryan Arnold wrote:

| Ok Andrew, here is draft3 of my patch.  This patch contains fixes for
| the following items:
| 
| 
| Thanks for the kthread suggestions.  The kthread API is awesome.  My
| stress tests seem to be going very well.  So, if you don't have any more
| comments....

I do.  (this is for the first 1000 lines of the patch... more to come)

+struct hvcs_partner_info {
+	/* list management */
+	struct list_head node;
+	/* partner unit address */
+	unsigned int unit_address;
+	/*partner partition ID */
+	unsigned int partition_ID;
+	/* CLC (79 chars) + 1 Null-term char */
+	char location_code[HVCS_CLC_LENGTH + 1];
+};

Ugly comments style.  Which comment goes with which
data?  Commenting data can be very helpful, but most of these
are close to useless since they are so obvious.
And put a space after "/*".

+/* Convert arch specific return codes into relevant errnos.  The hvcs
+ * functions aren't performance sensitive, so this conversion isn't an
+ * issue. */

Long-comment style is
/*
 * line1
 * line2
 * lineN
 */
(in multiple places).


+int hvcs_convert(long to_convert)
+{
+	switch (to_convert) {
+		case H_Success:
+			return 0;
+		case H_Parameter:
+			return -EINVAL;
+		case H_Hardware:
+			return -EIO;
+		case H_Busy:

Can these H_values be converted from that coding style?

+/* Helper function for hvcs_get_partner_info */
+int hvcs_next_partner(unsigned int unit_address, unsigned long last_p_partition_ID, unsigned long last_p_unit_address, unsigned long *pi_buff)

Split the function line. (multiple places)

+	memset(pi_buff,0x00,PAGE_SIZE);

Use spaces after commas.


+		/* This is a very small struct and will be freed soon */
+		next_partner_info = kmalloc(sizeof(struct hvcs_partner_info),
+				GFP_ATOMIC);

Where is it freed?

+	  will depend on arch specific apis exported from hvcserver.ko

"APIs"

+	  To compile this driver as a module, choose M here: the
+	  module will be called hvcs.ko.  Additionally, this module
+	  will depend on arch specific apis exported from hvcserver.ko
+	  which will also be compiled when this driver is built as a
+	  module.
+
 config PC9800_OLDLP

This patch segment won't apply since PC9800 has been removed.

+#define __ALIGNED__	__attribute__((__aligned__(8)))

Why aligned? why 8?  (just curious)  Could use a comment if it's important.



+			 * we commited to delivering it.  But don't try to wake
+			 * a non-existant tty. */

				non-existent


+	/* remove the read masks*/
			   masks */

+		for (i=0;got && i<got;i++)

	add spaces for readability:
		for (i = 0; got && i < got; i++)

+	if (!got){
	if (!got) {



--
~Randy