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

From: Randy.Dunlap
Date: Tue Jul 27 2004 - 18:16:19 EST


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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/