Re: [PATCH] ipwireless: driver for 3G PC Card

[David Sterba <dsterba@suse.cz>]

On Monday 28 of January 2008 19:08:54 Randy Dunlap wrote:

> > +/* Number of bytes in NL packet header (can not do
>
> cannot

Fixed.

>
> > + * sizeof(nl_packet_header) since it's a bitfield) */
> > +#define NL_FOLLOWING_PACKET_HEADER_SIZE    1
> > +
> > +struct nl_first_paket_header {
>
>                    packet ?

Yes, packet of course.

>
> > +#if defined(__BIG_ENDIAN)
> > +	unsigned char packet_rank:2;
> > +	unsigned char address:3;
> > +	unsigned char protocol:3;
> > +#else
> > +	unsigned char protocol:3;
> > +	unsigned char address:3;
> > +	unsigned char packet_rank:2;
> > +#endif
>
> From C99 spec:
> "The order of allocation of bit-fields within a unit (high-order to
> low-order or low-order to high-order) is implementation-defined."
>
> so if the order/location of these bitfields is important (from one
> system to another), you should use bit masks instead of bitfields
> for them.

I've changed it to __BIG_ENDIAN_BITFIELDS, as suggested by Alexey. The order 
is important. I'll add it to my todo to convert it to bitmasks.

> > +/*
> > + * @return 1 if something has been received from hw
>
> What's with the '@'?

Removed, and converted to plaintext as suggested by others.

> Need ":" and/or space between CARD_NAME and following string.
> (in several places)

Found a few of them and fixed.

> > +module_param(major, int, 0);
> > +module_param(ipwireless_debug, int, 0);
> > +module_param(ipwireless_loopback, int, 0);
> > +module_param(ipwireless_out_queue, int, 0);
> > +MODULE_PARM_DESC(major, "ttyIPWp major number [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "switch on debug messages [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "switch on loopback mode [0]");
> > +MODULE_PARM_DESC(ipwireless_debug, "set size of outgoing queue [1]");
>
> Will these parameters be documented anywhere?

The options are intended for debugging. Normal user does not need to tweak 
them. Adjusted the description to reflect their debugging purpose.

> > +#ifdef IPWIRELESS_STATE_DEBUG
> > +int ipwireless_dump_network_state(char *p, struct ipw_network *network)
> > +{
> > +	int idx = 0;
> > +
> > +	idx += sprintf(p + idx, "debug: ppp_blocked=%d\n",
> > +			network->ppp_blocked);
> > +	idx += sprintf(p + idx, "debug: outgoing_packets_queued=%d\n",
> > +			network->outgoing_packets_queued);
> > +	idx += sprintf(p + idx, "debug: network.shutting_down=%d\n",
> > +			network->shutting_down);
>
> check for overflow of 'p'?

Converted to one snprintf too.

> > +/* Syncronous start-messages */
>
> 	Synchronous

Fixed.

Updated patch will follow.


Thank you for comments.

Dave