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

[Ryan Arnold]

On Tue, 2004-07-27 at 17:50, Randy.Dunlap wrote:
> +struct hvcs_partner_info {};
> 
> 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 "/*".

Right, this is definitely more obvious now.  I think I can pretty much
remove the comments now that the element names make sense.

> +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?

Converted to what/how?  I am confused by your question.

> 
> +		/* 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?
> 
It is freed in hvcs_free_partner_info() because the partner info that is
allocated needs to have scope outside of the hvcs_get_partner_info()
call.

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

I'll make future patches against 2.6.8-rc2.

> +#define __ALIGNED__	__attribute__((__aligned__(8)))
> 
> Why aligned? why 8?  (just curious)  Could use a comment if it's important.

Randy, here's an explanation given by Hollis Blanchard and Paul
Mackerras that I'll add to the code as a comment.

The hcall interface involves putting 8 chars into each of two registers.
We load up those 2 registers (in arch/ppc64/hvconsole.c) by casting
char[16] to long[2].  It would work without __ALIGNED__, but a little
(tiny) bit slower because an unaligned load is slower than aligned load.

I took care of all the other formatting things you pointed out.

Thanks for the suggestions Randy.  Hopefully I'll have a patch out this
afternoon encompassing your suggestions.

Ryan S. Arnold
IBM Linux Technology Center

-
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/