Re: [PATCH 4/6] net: add NL802154 interface for configuration of 802.15.4 devices

[Johannes Berg <johannes@sipsolutions.net>]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

On Wed, 2009-06-03 at 16:27 +0400, Dmitry Eremin-Solenikov wrote:

> > > +/* commands */
> > > +/* REQ should be responded with CONF
> > > + * and INDIC with RESP
> > > + */
> > > +enum {
> > 
> > kernel-doc explaining the commands would be immensely helpful.
> 
> What explanations whould you like to see? These commands are described
> in the IEEE 802.15.4 standard.

Well, for example which arguments they require...

> > > +#ifdef __KERNEL__
> > > +struct net_device;
> > > +
> > > +int ieee802154_nl_assoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 cap);
> > > +int ieee802154_nl_assoc_confirm(struct net_device *dev, u16 short_addr, u8 status);
> > > +int ieee802154_nl_disassoc_indic(struct net_device *dev, struct ieee802154_addr *addr, u8 reason);
> > > +int ieee802154_nl_disassoc_confirm(struct net_device *dev, u8 status);
> > > +int ieee802154_nl_scan_confirm(struct net_device *dev, u8 status, u8 scan_type, u32 unscanned,
> > > +		u8 *edl/*, struct list_head *pan_desc_list */);
> > > +int ieee802154_nl_beacon_indic(struct net_device *dev, u16 panid, u16 coord_addr); /* TODO */
> > > +#endif
> > 
> > Why not just use two header files, one in net/ and one in linux/?
> 
> What would you suggest to put into the linux/ header and what in the
> net/ one?

userspace vs. in-kernel separation

> > > +static int ieee802154_nl_put_failure(struct sk_buff *msg)
> > > +{
> > > +	void *hdr = genlmsg_data(NLMSG_DATA(msg->data)); /* XXX: nlh is right at the start of msg */
> > > +	genlmsg_cancel(msg, hdr);
> > > +	nlmsg_free(msg);
> > > +	return -ENOBUFS;
> > > +}
> > 
> > This seems weird.
> 
> Why?

Because it's strange to cancel then free?

> > > +static int ieee802154_associate_req(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > +	struct net_device *dev;
> > > +	struct ieee802154_addr addr;
> > > +	int ret = -EINVAL;
> > > +
> > > +	if (!info->attrs[IEEE802154_ATTR_CHANNEL]
> > > +	 || !info->attrs[IEEE802154_ATTR_COORD_PAN_ID]
> > > +	 || (!info->attrs[IEEE802154_ATTR_COORD_HW_ADDR] && !info->attrs[IEEE802154_ATTR_COORD_SHORT_ADDR])
> > > +	 || !info->attrs[IEEE802154_ATTR_CAPABILITY])
> > > +		return -EINVAL;
> > 
> > That's some odd coding style.
> 
> Could you please elaborate this? What seems odd to you?

if (X
    && Y)

vs

if (X &&
    Y)

where the latter is used for all other sources files in the kernel. Try
applying that to _all_ your patches while you rework the sources to fit
into 80 columns.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]