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:
signature.asc
Description: This is a digitally signed message part
