Re: [RFC] Host Virtual Serial Interface driver

[Hollis Blanchard <hollisb@us.ibm.com>]

On Monday 09 August 2004 12:51, Randy.Dunlap wrote:
> 
> To the extent possible, we like to put the linux/* files in alpha
> order, and same with the asm/* files.  Separately, as you have them.

Ok.

> | #define __ALIGNED__	__attribute__((__aligned__(sizeof(long))))
>
> You should explain this bit (__ALIGNED__).

Ok.

> | static inline int hdrlen(const uint8_t *packet)
> | {
> | 	const int lengths[] = { 4, 6, 6, 8, };
> | 	struct hvsi_header *header = (struct hvsi_header *)packet;
> |
> | 	return lengths[VS_DATA_PACKET_HEADER - header->type];
> | }
>
> Any chance of bad data (value) in header->type ?

No, but I realized that was only being called from one place anyways, so this 
is simpler:

--- drivers/char/hvsi.c.lkml1   2004-08-09 13:34:22.000000000 -0500
+++ drivers/char/hvsi.c 2004-08-09 13:49:08.000000000 -0500
@@ -211,29 +211,11 @@
        spin_unlock_irqrestore(&hp->lock, flags);
 }

-static inline int hdrlen(const uint8_t *packet)
-{
-       const int lengths[] = { 4, 6, 6, 8, };
-       struct hvsi_header *header = (struct hvsi_header *)packet;
-
-       return lengths[VS_DATA_PACKET_HEADER - header->type];
-}
-
-static inline const uint8_t *payload(const uint8_t *packet)
-{
-       return packet + hdrlen(packet);
-}
-
 static inline int len_packet(const uint8_t *packet)
 {
        return (int)((struct hvsi_header *)packet)->len;
 }

-static inline int len_payload(const uint8_t *packet)
-{
-       return len_packet(packet) - hdrlen(packet);
-}
-
 static inline int is_header(const uint8_t *packet)
 {
        struct hvsi_header *header = (struct hvsi_header *)packet;
@@ -443,8 +425,9 @@
 static struct tty_struct *hvsi_recv_data(struct hvsi_struct *hp,
                const uint8_t *packet)
 {
-       const uint8_t *data = payload(packet);
-       int datalen = len_payload(packet);
+       const struct hvsi_header *header = (const struct hvsi_header *)packet;
+       const uint8_t *data = packet + sizeof(struct hvsi_header);
+       int datalen = header->len - sizeof(struct hvsi_header);
        int overflow = datalen - TTY_THRESHOLD_THROTTLE;

        pr_debug("queueing %i chars '%.*s'\n", datalen, datalen, data);

(And yes, header->len is guaranteed to be ok by the got_packet() check in the 
caller.)

> | 		if (hangup) {
> | 			tty_hangup(hangup);
> | 		}
>
> extra braces (style); maybe in a few other places also.

Ok.

-- 
Hollis Blanchard
IBM Linux Technology Center